htslib icon indicating copy to clipboard operation
htslib copied to clipboard

sam_format1 : replace if/then with switch/case

Open lindenb opened this issue 6 years ago • 1 comments

some minor optimizations :

in sam.c / sam_format1

I replaced the successions of if/then with switch/case.

replaced the goto with a macro.

I also fixed a | FIXME` in the same function.

FIXME: for better performance, put the loop after "if"

lindenb avatar May 24 '18 17:05 lindenb

I've had mixed results with this. For an uncompressed input file with no B-type tags, this PR is about 10% slower on an old Sandy Bridge-era CPU compared to develop. On a newer Broadwell core, it's about 5% faster. Looking at the resulting assembly code, the case statement got implemented as an indirect jump. Presumably the Broadwell core is better at branch predicting this than the Sandy Bridge one.

For the same file with the addition of B tags containing 64 uint16_t values, this pull request is about 3% faster on Sandy Bridge and 6% faster on Broadwell, so it looks like fixing the FIXME did have some effect.

Switch statements can be implemented in a number of different ways including an if-else if-else chain. It's not always easy to see if it's going to be faster, or if the compiler is making the best choice about which cases to try first. It might be better to try reordering the existing code so that the most common types seen "in the wild" are tried first, although I expect the gains will be small.

I don't really like replacing the goto with a macro. It adds more bloat to an already quite bloated function. Macros also make debugging harder (try debugging problems in khash...) Using goto is a perfectly good way of dealing with error conditions in C functions. See http://koblents.com/Ches/Links/Month-Mar-2013/20-Using-Goto-in-Linux-Kernel-Code/ for pro-goto arguments.

Your editor it using tabs. Please could you replace them with four spaces. We avoid tabs in htslib as no-one could agree on how long they should be.

daviesrob avatar May 25 '18 14:05 daviesrob