Skip to content

Conversation

@Ken-Patrick
Copy link
Contributor

getTokenArgumentFunction can be called many many times, and the vector
is not needed, but it can become quite costly.
This patch replaces getArguments with a function simply returning the
position of the token, if it is found in the arguments (thus saving the
cost of std::vector).

…ction

getTokenArgumentFunction can be called many many times, and the vector
is not needed, but it can become quite costly.
This patch replaces getArguments with a function simply returning the
position of the token, if it is found in the arguments (thus saving the
cost of std::vector).
@pfultz2
Copy link
Contributor

pfultz2 commented Aug 31, 2021

Do you still see 2x improvement with cppcheck with this change?

@Ken-Patrick
Copy link
Contributor Author

Do you still see 2x improvement with cppcheck with this change?

I probably was a bit optimistic about 2x, probably more like 1.5x (at least on the file I tested, I will test more thoroughly if I can find the time), so yes clear improvement (and std::vector disappears from my callgrind profile).

@pfultz2
Copy link
Contributor

pfultz2 commented Aug 31, 2021

I probably was a bit optimistic about 2x, probably more like 1.5x (at least on the file I tested, I will test more thoroughly if I can find the time), so yes clear improvement (and std::vector disappears from my callgrind profile).

Ok, I see 1.5x improvement as well for release build. The debug build is only about 7% faster.

@Ken-Patrick
Copy link
Contributor Author

Ok, I see 1.5x improvement as well for release build. The debug build is only about 7% faster.

My measurements were on release build. I didn't try in debug. ;-)

@danmar danmar merged commit 4296859 into danmar:main Sep 1, 2021
@danmar
Copy link
Owner

danmar commented Sep 1, 2021

My measurements were on release build. I didn't try in debug.

great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants