SmallVector #1912
SmallVector #1912
Conversation
|
@jgravelle-google, maybe you'd like to review this? no worries if not. |
src/support/small_vector.h
Outdated
| @@ -0,0 +1,164 @@ | |||
| #include <iostream> | |||
| template<typename T, size_t N> | ||
| struct SmallVector { | ||
| // fixed-space storage | ||
| size_t usedFixed = 0; |
|
|
||
| void pop_back() { | ||
| if (flexible.empty()) { | ||
| usedFixed--; |
|
|
||
| T& back() { | ||
| if (flexible.empty()) { | ||
| return fixed[usedFixed - 1]; |
| const SmallVector<T, N>* parent; | ||
| size_t index; | ||
|
|
||
| Iterator(const SmallVector<T, N>* parent, size_t index) : parent(parent), index(index) {} |
There was a problem hiding this comment.
Follow random access iterator: https://en.cppreference.com/w/cpp/named_req/RandomAccessIterator? e.g. use difference_type instead of int etc.
There was a problem hiding this comment.
Thanks, changing. Although I'm not sure it's useful to add all of those until they are tested, so just adding what is currently used.
|
|
||
| // Stacks of expressions tend to be limited in size (although, sometimes | ||
| // super-nested blocks exist for br_table). | ||
| typedef SmallVector<Expression*, 10> ExpressionStack; |
There was a problem hiding this comment.
Why 10? I'd think 4 or 8 would be more cache-friendly, though I guess it depends on how big std::vector is
There was a problem hiding this comment.
10 pointers on 64-bit would be a multiple of 16, so seems like it should be ok cache-wise, or am I missing something?
I kind of picked 10 because I didn't see a benchmark advantage to smaller or larger values, and it seems reasonable as a guess for a "typical" wasm function body.
|
For testing, maybe there's a generic unit test suite for vectors in C++? (quick search doesn't turn much up) Otherwise I can write some tests, but I'm sure it wouldn't be comprehensive. (This is tested by being used in the codebase, at least at a functional level.) |
|
I don't know of any test suites you can use, but there's enough code here that having some simple tests would be useful. The existing usage in the codebase likely doesn't stress the edge cases, so a future user might get tripped up. |
|
Makes sense. Ok, added a testcase file with some corner cases with the template parameter being 0, and crossing and uncrossing the boundary between the fixed and flexible storage. |
|
Thanks @binji and @jgravelle-google for the feedback here! |
Trying to refactor the code to be simpler and less redundant, I ran into some perf issues that it seems like a small vector, with fixed-size storage and optional additional storage as needed, might help with. This implements that class and uses it in a few places.
This seems to help, I see some 1-2% fewer instructions and cycles in
perf stat, but it's hard to tell if it really makes a noticeable difference.