Uh oh!
There was an error while loading. Please reload this page.
astutils.h: use pre-sized SmallVector in visitAstNodes() - #3919
Conversation
The Visual Studio compilation failure was caused by the incorrect C++ standard being using by the I opened #3937 for the compilation/selfcheck fixes. |
e83f0fa to
ea91209Comparefirewave
commented
May 28, 2022
There's another intermediate step it seems. std::vector<T *> tokensContainer;
tokensContainer.reserve(8);
std::stack<T *, std::vector<T *>> tokens(std::move(tokensContainer));Unfortunately that causes a performance regression when using Boost so that code requires to be conditional which is messy. I wonder if what the I provided values for the various cases in the description. |
firewave
commented
May 31, 2022
As |
With GCC I still see (all?) calls to |
SmallVector in visitAstNodes()firewave
commented
Aug 23, 2022
This is almost done. 🙂 Here's a small preview of the performance win: Clang 14 |
firewave
commented
Sep 8, 2022
I enabled the Boost usage for the sanitizer builds. That tests the build and runs the tests as well as (hopefully) providing a slight performance improvement for the sanitized runs. The last remaining thing is to build the Windows version with Boost. |
b76d0f8 to
fe25c15Comparefirewave
commented
Sep 28, 2022
This is finally ready for review. 🥳 I think the very slight Clang regression without Boost can be dismissed.
Performing a With Clang 14 With Valueflow enabled: Clang 14 And scanning Clang 14 |
This is the first of two PR introducing the usage of
SmallVectorin our code.I adjusted the value locally based on profiling it with
callgrind.When using Clang and Boost the indicator that we need to increase the size of the vector are
boost::container::vec_iterator<> boost::container::vector<>::priv_forward_range_insert_no_capacity<>(...)andoperator delete(void*)calls infindAstNode(...).When using no Boost and
reserve()the indicator is amount of additionaloperator new(...)andoperator delete(...)compared to the actual calls offindAstNode(...).Update: With
findAstNode()now being inlined it is now harder to trace this. You now need to follow theoperator new()oroperator delete()but you won't have a function or proper call count to compare against. It seem likememcpy@GLIBC_2.2.5is now the indicator that the vector is being expanded and needs to reserve more space.I still need to run a bigger test to see if we might need to increase the count even more.Here's some numbers for a-O2build usingcallgrindwith--enable=all --inconclusiveon themame_regtestproject,DISABLE_VALUEFLOW=1.Clang 132,263,092,948-std::vector2,242,113,164-std::vector(reserve(8))2,263,074,159-SmallVector(no Boost / size 8)2,242,113,077-SmallVector(no Boost / size 8 /reserve(8))2,162,604,179-SmallVector(Boost 1.74 / size 8)2,167,203,248-SmallVector(Boost 1.74 / size 8 /reserve(8))GCC values coming soon.I will file a ticket about the GCC difference soon.Updated numbers below.