[Perf optimization] Use boost::container::small_vector

[Ken-Patrick]

This appears to be quite beneficial (cppcheck gets twice as fast on
some files).

This is a first proposal, it is not supposed to be merged as it is now (it is probably missing some dependencies in the CI, among other things).
I used boost::container::small_vector, but maybe there are other solutions here, especially if we don't want to add a dependency to boost: copying llvm/ADT/SmallVector.h ?, something else ?

[pfultz2]

I wonder if small_vector could be implemented using custom allocators, which would also allow for smalll_map or small_set.

[pfultz2]

Actually this may be of interest:

https://howardhinnant.github.io/stack_alloc.html

[danmar]

cppcheck gets twice as fast on some files

sweet!! 👍

Actually this may be of interest

yes. I'd rather not add a boost dependency.

[Ken-Patrick]

Actually this may be of interest

yes. I'd rather not add a boost dependency.

I thought so. ;-)
stack_alloc looks promising indeed, I'll try to give it a try.

[Ken-Patrick]

Looking a bit more into this, it seems getTokenArgumentFunction is called many many times, hence the impact I saw from the cost of std::vector (in the call of getArguments in getTokenArgumentFunction)
However, all getTokenArgumentFunction wants to know is the position of a given argument (token) in a function call. So I guess I'll first try to rewrite this without using a container.

I don't know if std::vector will still be a little bottleneck after that.

[danmar]

However, all getTokenArgumentFunction wants to know is the position of a given argument (token) in a function call. So I guess I'll first try to rewrite this without using a container.

👍

[Ken-Patrick]

Done in #3435

[pfultz2]

There are actually a lot of other places where we return vectors of size 1 or 2(such as Analyzer::evaluate or Interval member fields) which seems like they could benefit from a small_vector class.

[Ken-Patrick]

There are actually a lot of other places where we return vectors of size 1 or 2(such as Analyzer::evaluate or Interval member fields) which seems like they could benefit from a small_vector class.

Probably. I played a bit with stack_alloc, it is not very convenient for that, at least a lot less than SmallVector (from llvm or boost): you can't return simply return the vector<short_alloc> because you also need to return the arena, and it is non copyable. So you have to do one of the following:

1. pass the arena into the function returning the vector,
2. use a (template) class containing the arena and the vector, and make the arena copyable/movable which sound like a pain... but then we could simply return the vector
3. don't return the vector, but instead pass it as a reference to the function currently returning it
4. I'm probably missing something

[pfultz2]

I see, I was thinking the arena was stored in the allocator. I did find this simple small_vector implementation that uses a custom allocator and then inherits from std::vector so it can initialize the allocator:

https://github.com/KonanM/small_vector/blob/master/include/small_vector/small_vector.h

I think it requires C++17, but it looks like it could be easily adapted to C++11.

[pfultz2]

I actually found this library:

https://github.com/alandefreitas/small

Its header-only(so it should be easy to copy into our external directory). However, this requires C++17. We could create type aliases that fallback on STL containers when using older compilers if we want to use such a library.

[pfultz2]

Also, caffe2 uses a copy of LLVM's SmallVector class but removes dependencies on extra LLVM constructs:

https://github.com/pytorch/pytorch/blob/master/c10/util/SmallVector.h

We would just need to define AlignedCharArrayUnion and C10_IS_TRIVIALLY_COPYABLE to make it completely standalone.

[danmar]

Its header-only(so it should be easy to copy into our external directory). However, this requires C++17. We could create type aliases that fallback on STL containers when using older compilers if we want to use such a library.

for information, I could allow something like:

#ifdef OLD_COMPILER
using smallvector=std::vector;
#else
using smallvector= SmallVector;
#endif

I want that it works to use old compilers but if some optimisation or "side feature" is lost that is fine.

[firewave]

I propose adding at least the alias and use it for cases where the container is slow so it is implicitly documented. I also suggest using a separate header file for this instead of adding it to an existing one so it is only includes in places where it is actually needed.

In CMake we could make Boost optional so it is only used when found and add it via a define as suggested by Danial. This would not add a hard dependency and we would not need to change any other build systems for now (as they might go away in the future anyways). Later we can replace it with a smaller and hard dependency.

This would be a very small change.

Also please profile the code before and after so we only use it in places it is actually needed.

[danmar]

not a hard dependency => sounds good to me.

[firewave]

#3799 adds the SmallVector alias with optional boost::container::small_vector usage.

I have a follow-up which switches the most obvious offender over to it.

Feel free to adjust other instances afterwards. Please profile before and afterwards so we don't switch over unnecessary usage.

[firewave]

Now that SmallVector is in I posted the first draft PR with usage of it - see #3919.

I have another one prepared but that needs some cleanups which cause performance regressions which I need to investigate (and possibly report to the compilers) first.

Will be a few days until I have both completely ready.

[firewave]

I posted a draft for another usage of SmallVector as #3925. During profiling I did not come across any other places where we actually had to replace the std::vector. If you have a case where the additional adjustments seen in this PR are necessary please provide that for further analysis. Thanks.

[firewave]

I did an intermediate improvement by pre-sizing the std::vector in visitAstodes() without any need for an external dependency for now - see #4158. It doesn't get us there completely but it is a start and all users will profit from it.

SmallVector still needs some work so it will have this advantage with the non-Boost implementation.

[firewave]

FYI #3919 is close to being ready for review.

[firewave]

Closing as #3919 and #3925 are finally ready for review now.