Skip to content

inline visitAstNodes() calls#3828

Merged
danmar merged 2 commits into
cppcheck-opensource:mainfrom
firewave:visitast
Feb 13, 2022
Merged

inline visitAstNodes() calls#3828
danmar merged 2 commits into
cppcheck-opensource:mainfrom
firewave:visitast

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

See https://blog.demofox.org/2015/02/25/avoiding-the-performance-hazzards-of-stdfunction/.

Testing with cli/threadexecutor.cpp:
GCC 11 656,137,737 -> 582,567,111
Clang 13 605,911,420 -> 556,949,984

@danmar danmar merged commit 75b6e8d into cppcheck-opensource:main Feb 13, 2022
@firewave firewave deleted the visitast branch February 13, 2022 22:26
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Jun 3, 2022

The inlining seems to behave differently with GCC since I still see calls to the function in question. There's also still a lot of std::function related overhead. Probably applies to #4146 as well. This will require some serious looking into to get it all the way like with Clang.

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Jun 4, 2022

I think its better to use a template parameter instead of std::function wherever possible. Not only will we have better performance, but its easier to step through with the debugger.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Jun 4, 2022

I think its better to use a template parameter instead of std::function wherever possible. Not only will we have better performance, but its easier to step through with the debugger.

In the profiler it badly messed things up since it is being inlined. It's really annoying. But you still might get to the source of things.

In GCC somehow this doesn't work as expected. I haven't filed as much reports with GCC as Clang yet and this really needs some looking into but that will probably take quite a while for me to get to...

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