GH-35820: [C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-win #35834
Conversation
westonpace
requested review from
AlenkaF,
assignUser,
domoritz,
kou,
lidavidm,
paleolimbot,
raulcd,
thisisnic,
trxcllnt,
wjones127 and
zeroshade
as code owners
|
|
|
|
github-actions
bot
added
Component: C#
Component: C++
Component: Gandiva
Component: Documentation
Component: FlightRPC
Component: GLib
Component: Go
Component: Java
Component: JavaScript
Component: MATLAB
Component: Parquet
Component: Python
Component: R
Component: Ruby
awaiting committer review
…ned allocations greater than 128 bytes and less than 1024 bytes. Our test was doing this but we never actually need this in our library code itself. So, instead, I changed the test to align to 128 bytes which still achieves the purpose of the test.
westonpace
force-pushed
the
bugfix/GH-35820--mimalloc-alignment-bug
branch
from
ab4c9ec to
3774325
Compare
westonpace
removed request for
AlenkaF,
assignUser,
paleolimbot,
raulcd,
thisisnic and
wjones127
|
Apologies for the over-zealous labeling. I was originally rebased off the maint branch. I've adjusted to the main branch. |
|
@github-actions crossbow submit test-build-vcpkg-win |
|
Revision: 3774325 Submitted crossbow builds: ursacomputing/crossbow @ actions-f7be1360b7
|
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
This certainly looks good enough to me.
…win (#35834) ### Rationale for this change There is a bug in the version of mimalloc that we currently vendor (2.0.6) which is microsoft/mimalloc#700 This bug causes aligned allocations to be improperly aligned if the requested alignment is greater than 128 and less than 1024. The allocations are always given 128 byte alignment. This also only seems to affect release mode. In practice, we never actually request alignment greater than 128 bytes. However, there was a test case that was requesting alignment of 256 bytes. Since this bug only affects a test case I'm not sure it warrants upgrading the mimalloc version (though we might want to do so at some point for other reasons). One could argue that memory pool is a part of our public interface and so this is a bug in a public method (the ability to allocate an aligned buffer) though users are welcome to use a newer version of mimalloc on their own. ### What changes are included in this PR? The test is modified to request 128 byte alignment instead of 256 byte alignment ### Are these changes tested? I was able to reproduce the issue on my Linux system by compiling in release mode and using mimalloc. I verified that upgrading mimalloc to 2.1.0 prevented the bug. The fix itself is a test case and so the change is tested. ### Are there any user-facing changes? No. * Closes: #35820 Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
|
Benchmark runs are scheduled for baseline = 89b1ebe and contender = 95df6cc. 95df6cc is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
3 participants
Rationale for this change
There is a bug in the version of mimalloc that we currently vendor (2.0.6) which is microsoft/mimalloc#700
This bug causes aligned allocations to be improperly aligned if the requested alignment is greater than 128 and less than 1024. The allocations are always given 128 byte alignment. This also only seems to affect release mode.
In practice, we never actually request alignment greater than 128 bytes. However, there was a test case that was requesting alignment of 256 bytes. Since this bug only affects a test case I'm not sure it warrants upgrading the mimalloc version (though we might want to do so at some point for other reasons).
One could argue that memory pool is a part of our public interface and so this is a bug in a public method (the ability to allocate an aligned buffer) though users are welcome to use a newer version of mimalloc on their own.
What changes are included in this PR?
The test is modified to request 128 byte alignment instead of 256 byte alignment
Are these changes tested?
I was able to reproduce the issue on my Linux system by compiling in release mode and using mimalloc. I verified that upgrading mimalloc to 2.1.0 prevented the bug. The fix itself is a test case and so the change is tested.
Are there any user-facing changes?
No.