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

[Sofa.Helper] Fix and micro-optimize AdvancedTimer #2349

Merged
merged 6 commits into from
Sep 21, 2021

Conversation

alxbilger
Copy link
Contributor

@alxbilger alxbilger commented Sep 20, 2021

  • In ScopedAdvancedTimer, store the timer id as a real id rather than the std::string used to find the real id. This fixes a strange bug occurring with ScopedAdvancedTimer (which I am not able to explain).
  • In AdvancedTimer, use a map for constant complexity when searching an id, instead of a std::vector

[ci-depends-on https://github.com/sofa-framework/Compliant/pull/2]


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@alxbilger alxbilger added pr: clean Cleaning the code pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Sep 20, 2021
@alxbilger
Copy link
Contributor Author

[ci-build][with-all-tests]

@sofabot
Copy link
Collaborator

sofabot commented Sep 21, 2021

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

const char* message;
ScopedAdvancedTimer(const std::string& message);
ScopedAdvancedTimer( const char* message );
AdvancedTimer::IdStep m_id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is not the topic of the PR and it is more about general c++ coding practice but including thousand of dependencies lines to get something that in fact is an integer is really problematic. I wonder what kind of design pattern should be used instead of these inner typedefs.

@fredroy fredroy added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Sep 21, 2021
@fredroy fredroy merged commit f413793 into sofa-framework:master Sep 21, 2021
@guparan guparan added this to the v21.12 milestone Oct 1, 2021
@guparan guparan added the pr: backport todo This PR will be backported into the release preceeding its milestone. label Oct 25, 2021
guparan pushed a commit that referenced this pull request Oct 25, 2021
* [Sofa.Helper] Store id as an id rather than the message used to find the id

* [Sofa.Helper] Use map for constant complexity instead of a std::vector

* [SofaCore] Missing include

* [SofaCore] Missing include

* [SofaCore] Missing include

* [SofaCore] Missing include
guparan added a commit that referenced this pull request Oct 26, 2021
@guparan guparan removed the pr: backport todo This PR will be backported into the release preceeding its milestone. label Oct 26, 2021
@alxbilger alxbilger deleted the timers branch June 28, 2022 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: clean Cleaning the code pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants