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

Replace likely macro with C++20 attribute #1514

Closed
heinezen opened this issue Jun 12, 2023 · 9 comments · Fixed by #1516
Closed

Replace likely macro with C++20 attribute #1514

heinezen opened this issue Jun 12, 2023 · 9 comments · Fixed by #1516
Labels
good first issue Suitable for newcomers improvement Enhancement of an existing component lang: c++ Done in C++ code

Comments

@heinezen
Copy link
Member

heinezen commented Jun 12, 2023

We currently use gcc's __builtin_expect() macro aliased as unlikely and likely for static branch prediction. Primarily, this optimizes checks for parser errors. This optimizes performance for code paths which are only taken under very specific circumstances.

Example:

if (unlikely(not this->value.exists())) {
    throw InternalError{"fetched nonexisting value of member"};
}

Since C++20, the standard supports attributes that do the same thing. Additionally, they are compiler agnostic, so they should also work in clang or msvc. They should also be more readable since they don't directly wrap the condition.

Basically the code gets changed to this:

if (not this->value.exists()) [[unlikely]] {
    throw InternalError{"fetched nonexisting value of member"};
}

If you want to discover the codebase or really like mundane tasks, then this is an issue for you :)


Also take note of the same issue in the nyan repository: SFTtech/nyan#105

@heinezen heinezen added improvement Enhancement of an existing component lang: c++ Done in C++ code good first issue Suitable for newcomers labels Jun 12, 2023
@SamantaTarun
Copy link
Contributor

I'd like to work on it.

@heinezen
Copy link
Member Author

@tarunsamanta2k20 Sweet :) Tell us if you need any directions.

@derekfrogget
Copy link
Contributor

@tarunsamanta2k20 Are you still working on this? I would like to work on it if you are not doing it anymore.

@SamantaTarun
Copy link
Contributor

@heinezen should i replace all occurrences of likely and unlikely both?

@SamantaTarun
Copy link
Contributor

SamantaTarun commented Jun 16, 2023

@derekfrogget if you want you may do it for 'likely' only. I am doing this only for unlikely cases.

@heinezen
Copy link
Member Author

@tarunsamanta2k20 @derekfrogget both likely and unlikel can be replaced.

There's also the same issue for the nyan repo SFTtech/nyan#105 (i think you also wanted to do this @tarunsamanta2k20 , right?)

@heinezen heinezen reopened this Jun 17, 2023
@heinezen heinezen changed the title Replace (un)likely macro with C++20 attribute Replace likely macro with C++20 attribute Jun 17, 2023
@heinezen
Copy link
Member Author

now it's only likely thats still there

@SamantaTarun
Copy link
Contributor

SamantaTarun commented Jun 17, 2023

@tarunsamanta2k20 @derekfrogget both likely and unlikel can be replaced.

There's also the same issue for the nyan repo SFTtech/nyan#105 (i think you also wanted to do this @tarunsamanta2k20 , right?)

@derekfrogget (he) will do for likely. Ping him. He is also interested to work on it.

@heinezen
Copy link
Member Author

Thanks everybody, now only the nyan repo task is left SFTtech/nyan#105 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Suitable for newcomers improvement Enhancement of an existing component lang: c++ Done in C++ code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants