Uh oh!
There was an error while loading. Please reload this page.
ARROW-13812: [C++] Fix Valgrind error in Grouper.BooleanKey test - #11041
ARROW-13812: [C++] Fix Valgrind error in Grouper.BooleanKey test#11041lidavidm wants to merge 2 commits into
Conversation
lidavidm
commented
Aug 31, 2021
@github-actions crossbow submit test-conda-cpp-valgrind |
Revision: 16700058feb3a672d5187cbfbf434b556b3a2fe2 Submitted crossbow builds: ursacomputing/crossbow @ actions-807
|
There was a problem hiding this comment.
Will it overflow the buffer if num_rows is multiplier of 8?
There was a problem hiding this comment.
It should be OK (there's a typo here, it's uint32_t values so the temporary buffer is actually overallocated by a factor of 4x). But I've adjusted it anyways.
lidavidm
commented
Sep 1, 2021
@github-actions crossbow submit test-conda-cpp-valgrind |
Revision: 22cbfd915497a114b45035379b842ef214cd06d8 Submitted crossbow builds: ursacomputing/crossbow @ actions-808
|
There was a problem hiding this comment.
I wonder if it wouldn't be easier to just always zero-initialize the 8 last bytes in the temp buffer?
There was a problem hiding this comment.
In that case, since the temp buffer is reused quite a bit, we might as well just initialize the underlying buffer on allocation? It should be a fixed setup cost since one large buffer is allocated on creation of the grouper (TempVectorStack) and then slices of it (TempVectorHolder) are taken as scratch space.
pitrou
commented
Sep 1, 2021
@ursabot please benchmark |
Benchmark runs are scheduled for baseline = bb533fb and contender = c005c9b. Results will be available as each benchmark for each run completes. |
Essentially, this failure boils down to: when generating the array of uniques for booleans, we pack 8 bytes at a time into one byte. The bytes are packed from what turns out to be a scratch array allocated by TempVectorStack, which does not initialize its memory. So when we have a non-multiple-of-8 number of bytes, we may end up packing initialized bytes and uninitialized bytes together into a single garbage byte, resulting in Valgrind complaining. Closesapache#11041 from lidavidm/arrow-13812 Authored-by: David Li <li.davidm96@gmail.com> Signed-off-by: Yibo Cai <yibo.cai@arm.com>
Essentially, this failure boils down to: when generating the array of uniques for booleans, we pack 8 bytes at a time into one byte. The bytes are packed from what turns out to be a scratch array allocated by TempVectorStack, which does not initialize its memory. So when we have a non-multiple-of-8 number of bytes, we may end up packing initialized bytes and uninitialized bytes together into a single garbage byte, resulting in Valgrind complaining.