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.

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

ARROW-13635: [Packaging][Python] Define --with-lg-page for jemalloc in the arm manylinux builds #10940

Closed
wants to merge 2 commits into from

Conversation

kszucs
Copy link
Member

@kszucs kszucs commented Aug 16, 2021

No description provided.

@github-actions
Copy link

@kszucs
Copy link
Member Author

kszucs commented Aug 16, 2021

@github-actions crossbow submit wheel-manylinux--py39--arm64

@kszucs
Copy link
Member Author

kszucs commented Aug 16, 2021

@github-actions crossbow submit wheel-manylinux*arm64

@github-actions
Copy link

Revision: 54a33f1

Submitted crossbow builds: ursacomputing/crossbow @ actions-777

Task Status
wheel-manylinux2014-cp36-arm64 TravisCI
wheel-manylinux2014-cp37-arm64 TravisCI
wheel-manylinux2014-cp38-arm64 TravisCI
wheel-manylinux2014-cp39-arm64 TravisCI

@@ -71,6 +71,10 @@ echo "=== (${PYTHON_VERSION}) Building Arrow C++ libraries ==="
: ${VCPKG_FEATURE_FLAGS:=-manifests}
: ${VCPKG_TARGET_TRIPLET:=${VCPKG_DEFAULT_TRIPLET:-x64-linux-static-${CMAKE_BUILD_TYPE}}}

if [[ "$(uname -m)" == arm* ]] || [[ "$(uname -m)" == aarch* ]]; then
export ARROW_EXTRA_CMAKE_FLAGS="-DARROW_JEMALLOC_LG_PAGE=14"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining why this is necessary?

Copy link
Contributor

@cyb70289 cyb70289 Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be 12 (4K)? Or 16 (64K)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--with-lg-page=16 fixes issue on 64k page system. #10929 (comment)

I tested on 4k page system with the image built from this pr --with-lg-page=14, it work okay.

According to jemalloc/jemalloc#467 (comment), looks we can set --with-lg-page=16 and the binary should work on both 4k and 64k page arm64 system.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to 16, though the conda-forge recipe uses 14.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arm64 does support 4k/16k/64k page sizes. 16k is rarely used and looks deprecated AFAIK.

@pitrou pitrou requested a review from cyb70289 August 16, 2021 17:56
@kszucs
Copy link
Member Author

kszucs commented Aug 16, 2021

Waiting for verification from #10929 (comment)

@@ -71,6 +71,10 @@ echo "=== (${PYTHON_VERSION}) Building Arrow C++ libraries ==="
: ${VCPKG_FEATURE_FLAGS:=-manifests}
: ${VCPKG_TARGET_TRIPLET:=${VCPKG_DEFAULT_TRIPLET:-x64-linux-static-${CMAKE_BUILD_TYPE}}}

if [[ "$(uname -m)" == arm* ]] || [[ "$(uname -m)" == aarch* ]]; then
export ARROW_EXTRA_CMAKE_FLAGS="-DARROW_JEMALLOC_LG_PAGE=14"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--with-lg-page=16 fixes issue on 64k page system. #10929 (comment)

I tested on 4k page system with the image built from this pr --with-lg-page=14, it work okay.

According to jemalloc/jemalloc#467 (comment), looks we can set --with-lg-page=16 and the binary should work on both 4k and 64k page arm64 system.

@kszucs
Copy link
Member Author

kszucs commented Aug 17, 2021

@github-actions crossbow submit wheel-manylinux*arm64

@github-actions
Copy link

Revision: 6608943

Submitted crossbow builds: ursacomputing/crossbow @ actions-779

Task Status
wheel-manylinux2014-cp36-arm64 TravisCI
wheel-manylinux2014-cp37-arm64 TravisCI
wheel-manylinux2014-cp38-arm64 TravisCI
wheel-manylinux2014-cp39-arm64 TravisCI

@cyb70289
Copy link
Contributor

@kszucs, is the 64k page size wheel available to download? I would like to test on both 64k and 4k arm systems.

@kszucs
Copy link
Member Author

kszucs commented Aug 17, 2021

@cyb70289
Copy link
Contributor

Tested okay with both 4K and 64K page size Arm64 kernels. Thanks @kszucs !

@cyb70289 cyb70289 closed this in 973dbae Aug 18, 2021
@kszucs
Copy link
Member Author

kszucs commented Aug 18, 2021

@xhochy we should update the conda-forge recipe to set the ARROW_JEMALLOC_LG_PAGE cmake variable instead.

@cyb70289
Copy link
Contributor

cyb70289 commented Aug 19, 2021

@xhochy we should update the conda-forge recipe to set the ARROW_JEMALLOC_LG_PAGE cmake variable instead.

I find apple m1 is using 16K page size. As apple ecosystem is different from linux, looks we can keep current --with-lg-page=14 for m1?

EDIT: Ah, I see. We should append -DARROW_JEMALLOC_LG_PAGE=16 for arm64 linux to EXTRA_CMAKE_ARGS in file https://github.com/apache/arrow/blob/master/dev/tasks/conda-recipes/arrow-cpp/build-arrow.sh

@cyb70289
Copy link
Contributor

New pr #10963 to fix conda recipe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants