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 intoapache:masterfrom
Conversation
|
|
|
@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
force-pushed
the
arrow-13812
branch
from
1670005 to
22cbfd9
Compare
|
@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.
lidavidm
force-pushed
the
arrow-13812
branch
from
22cbfd9 to
c005c9b
Compare
|
@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. Closes apache#11041 from lidavidm/arrow-13812 Authored-by: David Li <[email protected]> Signed-off-by: Yibo Cai <[email protected]>
4 participants
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.