Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-win #35820

Closed
raulcd opened this issue May 30, 2023 · 1 comment · Fixed by #35834
Closed

[C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-win #35820

raulcd opened this issue May 30, 2023 · 1 comment · Fixed by #35834
Assignees
@westonpace
Labels
Component: C++ Priority: Blocker Marks a blocker for the release Type: bug
Milestone
12.0.1

Comments

@raulcd
Copy link
Member

raulcd commented May 30, 2023

Describe the bug, including details regarding any error messages, version, and platform.

Since #35565 was merged and EnsureAlignment.Buffer was added the job test-build-vcpkg-win has been failing with:

 [  FAILED  ] 1 test, listed below:
[  FAILED  ] EnsureAlignment.Buffer

 1 FAILED TEST
  YOU HAVE 1 DISABLED TEST

Component(s)

C++, Continuous Integration
@raulcd raulcd added this to the 12.0.1 milestone May 30, 2023
@raulcd
Copy link
Member Author

raulcd commented May 30, 2023

The error seems to be on the alignment itself:

D:/a/crossbow/crossbow/arrow/cpp/src/arrow/util/align_util_test.cc(204): error: Value of: util::CheckAlignment(*realigned_large, 256)
  Actual: false
Expected: true

cc @westonpace

@github-actions github-actions bot mentioned this issue May 31, 2023
Merged
pitrou pushed a commit that referenced this issue May 31, 2023
@westonpace
GH-35820: [C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-…
95df6cc 
…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>
@pitrou pitrou modified the milestones: 12.0.1, 13.0.0 May 31, 2023
@raulcd raulcd modified the milestones: 13.0.0, 12.0.1 May 31, 2023
raulcd pushed a commit that referenced this issue May 31, 2023
@westonpace @raulcd
GH-35820: [C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-…
3718c66 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@westonpace westonpace

Labels
Component: C++ Priority: Blocker Marks a blocker for the release Type: bug
Projects
None yet
Milestone
12.0.1
Development

Successfully merging a pull request may close this issue.

GH-35820: [C++][CI] EnsureAlignment.Buffer fails on test-build-vcpkg-win westonpace/arrow
3 participants
@raulcd @westonpace @pitrou