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

Reduce branching on a common case #11074

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

Conversation

gkorland
Copy link
Contributor

@gkorland gkorland commented Aug 2, 2022

When the prefix is ":" code takes a long path through all the branches and calculates the OBJ_SHARED_HDR_STRLEN for nothing.

@oranagra
Copy link
Member

oranagra commented Aug 2, 2022

I usually object this kind of optimization changes unless we prove they actually make a difference.
these are typical things that an optimizing compiler doesn't need you to tell it.
if we do have an advantage over the compiler knowing what's common and what isn't, we need to use __builtin_expect to instruct the compiler what's more likely.

that's true about using switch-case vs if-else, grouping multiple if's with a common condition into a nested one, and also about a computation that isn't always needed (or is placed inside a loop and can be moved out)
the compiler can realize all these things on its own and do that...
many times you can change the code and it'll not affect the binary.

testing it: https://godbolt.org/z/TzYxePKTd
we can see that both are generating an if-else chain list, not a jump lookup table.
e.g.

        cmp     dl, 42
        je      .L42
        jg      .L43
        cmp     dl, 36
        je      .L44
        cmp     dl, 37
        jne     .L40

indeed in yours it doesn't do OBJ_SHARED_HDR_STRLEN unconditionally, but i'll argue it's just by chance.
and also note that it's not really calling any computation, and doesn't even have a branch!

        xor     eax, eax
        cmp     rsi, 9
        setg    al
        add     rax, 4

i'd like to see a benchmark that proves it actually does something

@filipecosta90 filipecosta90 self-requested a review August 2, 2022 22:22
@filipecosta90 filipecosta90 added the action:run-benchmark Triggers the benchmark suite for this Pull Request label Aug 2, 2022
@gkorland
Copy link
Contributor Author

gkorland commented Aug 3, 2022

First I think the code is more readable this way.
But, also in the common case where user value ll is not restricted to 0-31 we avoid the all chain of ifs.

@oranagra
Copy link
Member

oranagra commented Aug 3, 2022

i agree with both statements. but i'm arguing that in some cases the compiler would have already done these on it's own (grouping similar conditions and moving computation around), and that without using __builtin_expect we can't be sure the compiler will comply to the changes we make.

in your case, with the one specific compiler i checked, it seems it did! but i'm generally against such micro optimizations without measuring a consistent improvement.

@filipecosta90 filipecosta90 added action:run-benchmark Triggers the benchmark suite for this Pull Request and removed action:run-benchmark Triggers the benchmark suite for this Pull Request labels Oct 8, 2022
@filipecosta90 filipecosta90 added action:run-benchmark Triggers the benchmark suite for this Pull Request and removed action:run-benchmark Triggers the benchmark suite for this Pull Request labels Jan 17, 2023
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action:run-benchmark Triggers the benchmark suite for this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants