Defer the call to get timer resolution until needed #2440
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
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
We have a use case where the highway library is linked into a shared object. Currently whenever it is dlopen'ed, the static initializer runs including running the `platform::TimerResolution`. This has been observed in profiling and raised some questions, namely it shouldn't be necessary to initialize the benchmarking code if we are using the highway library but not to benchmark anything. Furthermore, doing so is against many companies' C++ guidelines, making this a blocker for inclusion into companies' code bases. This commit changes this variable from a namespace-scope static variable into a static local variable. It may have a little bit more overhead but it should be negligible.
kccqzy
force-pushed
the
timer-resolution-defer-2
branch
from
84314ad to
cd56bbc
Compare
jan-wassenberg
approved these changes
Jan 14, 2025
There was a problem hiding this comment.
Thanks for the PR! I agree this is an improvement.
FWIW our Bazel build has nanobenchmark as a separate optional build target/dependency, but in CMake it's unfortunately bundled together with the hwy files.
I had a quick look for other non-local static objects and did not see any.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
We have a use case where the highway library is linked into a shared object. Currently whenever it is dlopen'ed, the static initializer runs including running the
platform::TimerResolution. This has been observed in profiling and raised some questions, namely it shouldn't be necessary to initialize the benchmarking code if we are using the highway library but not to benchmark anything.Furthermore, doing so is against many companies' C++ guidelines, making this a blocker for inclusion into companies' code bases.
This commit changes this variable from a namespace-scope static variable into a static local variable.