calamine

calamine copied to clipboard

https://github.com/tafia/calamine/blob/c80df57b4b0a088ebcc3a4f4ff11bcf61832649a/src/xls.rs#L316

According to the output file of objdump, this ? operator (which has been used to repladced try! macro since 1.39.0) will introduce some unnecessary memory operations. If we change it into let r = record.unwrap();, these memory operations will be eliminated.

Same with the ? in these lines: https://github.com/tafia/calamine/blob/c80df57b4b0a088ebcc3a4f4ff11bcf61832649a/src/xls.rs#L320-L328

With these modifications, according to my test with search_errors example, the average execution time decreased from 3.3s to 3.1s, which is a 1.06x speedup.

Hope this information helps!

In my opinion, a library should never panic. If there's something wrong with the file, I would rather it propagates all the way to the user even if it means more branches.

@yyzdtccjdtc are you sure you did compile with --release ? Also I don't think we want to panic/hard crash on invalid input..

@yyzdtccjdtc are you sure you did compile with --release ? Also I don't think we want to panic/hard crash on invalid input..

Yes, I compile it with --release and set opt-level to 3. According to my test, unwrap() and try() all will panic if the input is None, which means their output should be the same.

While I am surprised with the drop in performance using ? (thanks for reporting it), I agree with the others that I'd rather lose some performance but ensure I'm not panicking. That being said there may be some way to ensure that some of these ? could actually never fail.