-
Notifications
You must be signed in to change notification settings - Fork 35
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
Optimize log message code generation #1101
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sethrj
added
core
Software engineering infrastructure
performance
Changes for performance optimization
labels
Feb 2, 2024
pcanal
reviewed
Feb 2, 2024
pcanal
approved these changes
Feb 2, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pcanal
pushed a commit
to pcanal/celeritas
that referenced
this pull request
Feb 6, 2024
* Force-inline logger message functions * Pass provenance by rvalue * Use string view for provenance * Only inline non-null log message components * Mark logging as unlikely * Fix potential bug when moving logger message * Remove silly dynamic cast * Rename Provenance to LogProvenance
sethrj
added a commit
that referenced
this pull request
Feb 14, 2024
* Force-inline logger message functions * Pass provenance by rvalue * Use string view for provenance * Only inline non-null log message components * Mark logging as unlikely * Fix potential bug when moving logger message * Remove silly dynamic cast * Rename Provenance to LogProvenance
sethrj
added a commit
that referenced
this pull request
Feb 15, 2024
* Force-inline logger message functions * Pass provenance by rvalue * Use string view for provenance * Only inline non-null log message components * Mark logging as unlikely * Fix potential bug when moving logger message * Remove silly dynamic cast * Rename Provenance to LogProvenance
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
core
Software engineering infrastructure
enhancement
New feature or request
performance
Changes for performance optimization
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While testing the contents of
g4vg.dylib
withnm
to make sure no Celeritas physics was included, I found that template instantiations for the logger message onchar const[N]
were being saved as actual function calls. This is pretty silly because those functions unroll effectively toput_char(ptr, N)
, so there's not going to be any code size gain by not inlining those.The second optimization is that I've marked as "unlikely" that the log level is high enough to print, moving the printing code to a "cold" section. This improves instruction cache locality in the typical case where a logger message is ignored.
I've also replaced
std::string
withstd::string_view
and made better use of move semantics to further reduce the amount of code needed to create and destroy a logging message, especially when it's not printed.For reference, given the code:
With the new code, there are 15 instructions between "get the world logger" and the function exit. With the old code, there's ~120 (but the "not logging" case skips some of those). The old code also has several calls to
delete
due to usingstring
rather thanstring_view
.