Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sam_format1 : replace if/then with switch/case #704

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

lindenb
Copy link

@lindenb lindenb commented May 24, 2018

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"

@daviesrob
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants