-
Notifications
You must be signed in to change notification settings - Fork 3.9k
GH-47643: [Python][Packaging] Enable CMAKE_INTERPROCEDURAL_OPTIMIZATION for wheels #47733
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
Conversation
…MIZATION for wheels
|
@github-actions crossbow submit wheel-musllinux-1-2-cp313-cp313-amd64 wheel-manylinux-2-28-cp312-cp312-arm64 wheel-windows-cp311-cp311-amd64 |
Revision: 57a1587 Submitted crossbow builds: ursacomputing/crossbow @ actions-cf593156e6
|
@github-actions crossbow submit -g wheel |
Revision: 57a1587 Submitted crossbow builds: ursacomputing/crossbow @ actions-5510c5761f |
It seems we are getting ODR violations on musllinux when we enable ninja: job failed: : && /usr/bin/c++ -fPIC -Wno-noexcept-type -Wno-self-move -Wno-subobject-linkage -fdiagnostics-color=always -Wall -fno-semantic-interposition -msse4.2 -O3 -DNDEBUG -O2 -ftree-vectorize -flto=auto -fno-fat-lto-objects -Wl,--version-script=/arrow/cpp/src/arrow/symbols.map -shared -Wl,-soname,libarrow.so.2200 -o release/libarrow.so.2200.0.0 src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_3_cxx.cxx.o src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_2_cxx.cxx.o src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_1_cxx.cxx.o src/arrow/CMakeFiles/arrow_objlib.dir/Unity/unity_0_cxx.cxx.o src/arrow/CMakeFiles/arrow_array.dir/Unity/unity_2_cxx.cxx.o src/arrow/CMakeFiles/arrow_array.dir/Unity/unity_1_cxx.cxx.o src/arrow/CMakeFiles/arrow_array.dir/Unity/unity_0_cxx.cxx.o src/arrow/CMakeFiles/arrow_compute_core.dir/Unity/unity_3_cxx.cxx.o src/arrow/CMakeFiles/arrow_compute_core.dir/Unity/unity_2_cxx.cxx.o src/arrow/CMakeFiles/arrow_compute_core.dir/Unity/unity_1_cxx.cxx.o src/arrow/CMakeFiles/arro
/arrow/cpp/src/arrow/io/concurrency.h:162: warning: virtual table of type 'struct RandomAccessFileConcurrencyWrapper' violates one definition rule [-Wodr]
ninja: subcommand failed
162 | class RandomAccessFileConcurrencyWrapper : public RandomAccessFile {
|
/arrow/cpp/src/arrow/io/concurrency.h:162:7: note: the conflicting type defined in another translation unit
162 | class RandomAccessFileConcurrencyWrapper : public RandomAccessFile {
| ^
/arrow/cpp/src/arrow/io/interfaces.cc:108: note: virtual method 'io_context'
108 | const IOContext& Readable::io_context() const { return default_io_context(); }
|
<built-in>: note: ought to match virtual method '__cxa_pure_virtual' but does not
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: warning: virtual table of type 'struct VarLengthListLikeBuilder' violates one definition rule [-Wodr]
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: note: the conflicting type defined in another translation unit
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:241:16: note: virtual method 'UnsafeAppendEmptyDimensions'
241 | virtual void UnsafeAppendEmptyDimensions(int64_t num_values) {
| ^
<built-in>: note: ought to match virtual method '__cxa_pure_virtual' but does not
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: warning: virtual table of type 'struct VarLengthListLikeBuilder' violates one definition rule [-Wodr]
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: note: the conflicting type defined in another translation unit
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:241:16: note: virtual method 'UnsafeAppendEmptyDimensions'
241 | virtual void UnsafeAppendEmptyDimensions(int64_t num_values) {
| ^
<built-in>: note: ought to match virtual method '__cxa_pure_virtual' but does not
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: warning: virtual table of type 'struct VarLengthListLikeBuilder' violates one definition rule [-Wodr]
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: note: the conflicting type defined in another translation unit
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:241:16: note: virtual method 'UnsafeAppendEmptyDimensions'
241 | virtual void UnsafeAppendEmptyDimensions(int64_t num_values) {
| ^
<built-in>: note: ought to match virtual method '__cxa_pure_virtual' but does not
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: warning: virtual table of type 'struct VarLengthListLikeBuilder' violates one definition rule [-Wodr]
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:46:7: note: the conflicting type defined in another translation unit
46 | class VarLengthListLikeBuilder : public ArrayBuilder {
| ^
/arrow/cpp/src/arrow/array/builder_nested.h:241:16: note: virtual method 'UnsafeAppendEmptyDimensions'
241 | virtual void UnsafeAppendEmptyDimensions(int64_t num_values) {
| ^
<built-in>: note: ought to match virtual method '__cxa_pure_virtual' but does not
/usr/include/fortify/stdio.h: In function '__to_xstring.constprop':
/usr/include/fortify/stdio.h:73:28: error: inlining failed in call to 'always_inline' 'vsnprintf': function body can be overwritten at link time
73 | _FORTIFY_FN(vsnprintf) int vsnprintf(char * _FORTIFY_POS0 __s, size_t __n,
| ^
/usr/include/c++/13.2.1/ext/string_conversions.h:113:32: note: called from here
113 | const int __len = __convf(__s, __n, __fmt, __args);
| ^
make: *** [/tmp/ccICEbbk.mk:362: /tmp/ccGClDbC.ltrans120.ltrans.o] Error 1
make: *** Waiting for unfinished jobs....
lto-wrapper: fatal error: make returned 2 exit status
compilation terminated.
/usr/lib/gcc/x86_64-alpine-linux-musl/13.2.1/../../../../x86_64-alpine-linux-musl/bin/ld: error: lto-wrapper failed |
@pitrou should I just disable |
Oh, yes, please do :) |
@github-actions crossbow submit wheel-musllinux-1-2-cp313-cp313-amd64 wheel-manylinux-2-28-cp312-cp312-arm64 |
Revision: df0b2d9 Submitted crossbow builds: ursacomputing/crossbow @ actions-68709fa7c8
|
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} \ | ||
-DCMAKE_INSTALL_LIBDIR=lib \ | ||
-DCMAKE_INSTALL_PREFIX=/tmp/arrow-dist \ | ||
-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=${CMAKE_INTERPROCEDURAL_OPTIMIZATION} \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for C++, but can we also try passing it for Python using PYARROW_CMAKE_OPTIONS
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense, let me have all Arrow C++ builds with it correctly and I will try to add it to pyarrow afterwards
…y environment variable
@github-actions crossbow submit wheel-musllinux-1-2-cp313-cp313-amd64 wheel-manylinux-2-28-cp312-cp312-arm64 |
Revision: 4be6dc5 Submitted crossbow builds: ursacomputing/crossbow @ actions-3b75f5b645
|
@github-actions crossbow submit wheel-musllinux-1-2-cp313-cp313-amd64 wheel-manylinux-2-28-cp312-cp312-arm64 wheel-windows-cp311-cp311-amd64 |
Revision: 5b14294 Submitted crossbow builds: ursacomputing/crossbow @ actions-df10b76e78
|
Some examples of wheel sizes after the change, there's around 2% size reduction on the tested examples:
@pitrou is this in-line with what you were expecting from your earlier tests? |
@github-actions crossbow -g wheel |
|
@github-actions crossbow submit -g wheel |
Revision: 5b14294 Submitted crossbow builds: ursacomputing/crossbow @ actions-ac76853b88 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@raulcd Did you update the numbers in the PR description? |
No since I posted them originally. I downloaded the built artifact on the job (wheel.zip) and unzipped to compare, why? |
Ah, that was already after you changed |
Yes, that's with the current changes |
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 771e14b. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…MIZATION for wheels (apache#47733) ### Rationale for this change Experimentation in apache#47637 suggested that [enabling inter-procedural optimization (IPO)](https://cmake.org/cmake/help/latest/variable/CMAKE_INTERPROCEDURAL_OPTIMIZATION.html) could not only improve performance in some workloads, but also reduce the code size of Arrow libraries. There's around 2% size reduction on the tested examples: |wheel|size| |---------|--------| |last successful nightly `wheel-manylinux-2-28-cp312-cp312-arm64` | 44605918 bytes| |with change `wheel-manylinux-2-28-cp312-cp312-arm64` | 43807946 bytes| |last successful nightly `wheel-windows-cp311-cp311-amd64` | 28093080 bytes| |with change `wheel-windows-cp311-cp311-amd64` | 27391407 bytes| ### What changes are included in this PR? Add `-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON` to Arrow C++ and wheels for both manylinux and windows. ### Are these changes tested? Yes via CI ### Are there any user-facing changes? No * GitHub Issue: apache#47643 Authored-by: Raúl Cumplido <[email protected]> Signed-off-by: Raúl Cumplido <[email protected]>
Rationale for this change
Experimentation in #47637 suggested that enabling inter-procedural optimization (IPO) could not only improve performance in some workloads, but also reduce the code size of Arrow libraries.
There's around 2% size reduction on the tested examples:
wheel-manylinux-2-28-cp312-cp312-arm64
wheel-manylinux-2-28-cp312-cp312-arm64
wheel-windows-cp311-cp311-amd64
wheel-windows-cp311-cp311-amd64
What changes are included in this PR?
Add
-DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON
to Arrow C++ and wheels for both manylinux and windows.Are these changes tested?
Yes via CI
Are there any user-facing changes?
No