GH-47412: [C++] Use inlineshidden visibility in Meson configuration#47413
GH-47412: [C++] Use inlineshidden visibility in Meson configuration#47413kou merged 2 commits intoapache:mainfrom
Conversation
|
|
aa0d954 to
bf63670
Compare
cpp/src/arrow/flight/meson.build
Outdated
There was a problem hiding this comment.
The CMake configuration currently does not have this export macro, and it looks like the FlightDataDeserialize test currently has:
GTEST_SKIP() << "Can't use Protobuf symbols on Windows";So perhaps this should be ported to CMake as well? Not sure the extent of Flight support on Windows, but maybe @lidavidm has thoughts?
There was a problem hiding this comment.
IIRC every time I've tried, it's turned out that protoc doesn't actually insert the dllexport declarations in the right places and you get something unusable
There was a problem hiding this comment.
At least locally I see this inserted for each class in the generated file in the build folder
There was a problem hiding this comment.
I suppose if it works now we can port it over
There was a problem hiding this comment.
Great - I'll open a separate issue for that tomorrow
There was a problem hiding this comment.
See #47429 - my naieve attempt at a fix did not work. Looks like there are some more complicating factors with how gRPC and the Flight tests are getting linked...but in any case its there to track
271c0c1 to
408bea9
Compare
408bea9 to
3c06f4a
Compare
|
CC @kou |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 556c239. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
…tion (apache#47413) ### Rationale for this change I was under the false impression that 'hidden' visibility hid the most symbols from shared libraries. As it turns out, 'inlineshidden' performs the most symbol hiding, so we can achieve the best hygeine and smallest libraries from that option. A local release build shows the following library sizes on main: 21M src/arrow/libarrow-compute.so 16M src/arrow/libarrow.so 21M src/arrow/flight/libarrow-flight.so 2.5M src/arrow/acero/libarrow-acero.so With this PR showing a small improvement in some libraries: 21M src/arrow/libarrow-compute.so 16M src/arrow/libarrow.so 20M src/arrow/flight/libarrow-flight.so 2.4M src/arrow/acero/libarrow-acero.so ### What changes are included in this PR? Usage of 'hidden' has been replaced with 'inlineshidden'. I also added hidden symbol visibility to the compute library, where it was errantly missing before ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#47412 Authored-by: Will Ayd <william.ayd@icloud.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
I was under the false impression that 'hidden' visibility hid the most symbols from shared libraries. As it turns out, 'inlineshidden' performs the most symbol hiding, so we can achieve the best hygeine and smallest libraries from that option.
A local release build shows the following library sizes on main:
21M src/arrow/libarrow-compute.so
16M src/arrow/libarrow.so
21M src/arrow/flight/libarrow-flight.so
2.5M src/arrow/acero/libarrow-acero.so
With this PR showing a small improvement in some libraries:
21M src/arrow/libarrow-compute.so
16M src/arrow/libarrow.so
20M src/arrow/flight/libarrow-flight.so
2.4M src/arrow/acero/libarrow-acero.so
What changes are included in this PR?
Usage of 'hidden' has been replaced with 'inlineshidden'. I also added hidden symbol visibility to the compute library, where it was errantly missing before
Are these changes tested?
Yes
Are there any user-facing changes?
No