remove use of variable length arrays#4870
remove use of variable length arrays#4870mattklein123 merged 3 commits intoenvoyproxy:masterfrom greenhouse-org:stack-array
Conversation
Variable length arrays are not part of C++, so replace them with a macro that: 1. Uses alloca to allocate the memory on the stack 2. Wrap the buffer with a wrapper template class that handles constructing / destructing the elements of the array Add "-Wvla" to the POSIX copts Signed-off-by: Natalie Arellano <narellano@pivotal.io> Signed-off-by: Sam Smith <sesmith177@gmail.com>
|
Haven't looked at code yet, but if you are going to change the linux path we should probably get @mattklein123 's buy-in before even reviewing, and likely we'll want some perf tests showing the impact, a la #4867 |
Presumably what this code is doing with a wrapper around alloca() is essentially what I imagine gcc/clang are doing when generating VLA code, so I doubt there will be much/any perf difference. So at a high level it's fine with me. @jmarantz are you interested in taking a first pass on this? |
source/common/common/stack_array.h
Outdated
| template <typename T> class StackArray { | ||
| public: | ||
| StackArray(void* buf, size_t num_items) : num_items_(num_items) { | ||
| RELEASE_ASSERT(buf != nullptr, "StackArray received null pointer"); |
There was a problem hiding this comment.
This can be a normal ASSERT IMO
jmarantz
left a comment
jmarantz
left a comment
There was a problem hiding this comment.
This looks awesome. I just some trivial optimization suggestions and test nits.
|
|
||
| #if !defined(WIN32) | ||
| #include <alloca.h> | ||
|
|
There was a problem hiding this comment.
it looks like the tools/check_format.py fix script is adding this new line back
source/common/common/stack_array.h
Outdated
| StackArray(void* buf, size_t num_items) : num_items_(num_items) { | ||
| RELEASE_ASSERT(buf != nullptr, "StackArray received null pointer"); | ||
| buf_ = static_cast<T*>(buf); | ||
| for (size_t i = 0; i < num_items_; i++) { |
There was a problem hiding this comment.
as a micro-optimization (and to look more like STL impls I've peeked into) you could store end_ rather than num_items_. Then you'd loop with a range loop here and in the dtor.
for (T& ref : *this) {
new (&ref) T;
}
The micro-optimization is that there are now two arithmetic operation per loop (bumping the pointer and comparing to end) rather than three (incrementing the index and adding it to the base, and comparing to num_items_). Of course the compiler may very well be able to optimize it to the same instructions, but using the range-loop syntax seems like a benefit too :)
There was a problem hiding this comment.
Sorry that was confusing. This is really two independent suggestions:
- Use a range-loop for iterating in the ctor and dtor
- Now there are no references to num_items_ anywhere in your impl except for the implementation of end(), so just store end_ rather than num_items_.
source/common/common/stack_array.h
Outdated
| namespace Envoy { | ||
|
|
||
| // This macro is intended to be used as a replacement for variable-length arrays. | ||
| // Note that the StackArray wrapper object will be destructed when it leaves |
There was a problem hiding this comment.
also each element's dtor will be called.
| ~TestEntry() { destructor_(val_); } | ||
|
|
||
| int val_ = 0; | ||
| bool constructed_ = false; |
There was a problem hiding this comment.
this seems strange as there won't be a time when the test-method has control where constructed_ will be false.
I thought for a couple of minutes how to gain confidence your ctor got run. What you are doing below would also work if StackArray failed to construct anything, but the stack-memory had random garbage in it (which would be interpreted as truthy).
Maybe have a member-var TestEntry* self_, and init it in the ctor to this. Then you can check that happened.
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks this is a great cleanup. One question.
| // Note that the StackArray wrapper object will be destructed and each element's | ||
| // destructor will be called when it leaves scope. However, the memory containing | ||
| // the array won't be deallocated until the function containing the macro returns | ||
| #define STACK_ARRAY(var, type, num) StackArray<type> var(::alloca(sizeof(type) * num), num) |
There was a problem hiding this comment.
Thinking about this, do we actually need this macro? Couldn't we just specify the template instantiation directly with type and num for each var and actually have :alloca be done as part of the constructor? Or does that now work because then the alloca would be cleaned up on constructor exit? If that's the case do you mind adding a small comment to that effect?
There was a problem hiding this comment.
That's correct, if we called alloca in the constructor, the memory would be freed when the constructor returns
Signed-off-by: Sam Smith <sesmith177@gmail.com> Signed-off-by: Amin Jamali <ajamali@pivotal.io>
1. Upgrade Envoy. 2. Remove use of variable length arrays. See envoyproxy/envoy#4870
3 participants
Description:
After #4645 was merged, @jmarantz noticed that the
STACK_ALLOC_ARRAYmacro was incorrect on Windows -- it was not properly initializing the memory for the array and the destructors of any array elements would not get called. This PR fixes the macro to:We also update the Linux build to use this alloca+wrapper class strategy and so can add "-Wvla" to the POSIX copts to catch further uses of variable length arrays
Risk Level:
Low - should be no change in behavior
Testing:
bazel build //source/exe:envoy-static && bazel test //test/...Docs Changes:
N/A
Release Notes:
N/A