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

Add option to build monolithic, shared library #10732

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

assignUser
Copy link
Collaborator

@assignUser assignUser commented Aug 13, 2024

This introduces the option to build a shared version of the monolithic libvelox.a, the result is a much smaller build tree, library and executables. They are now so small that moving them is much easier, e.g. for test sharding.

I also fixed some miscellaneous things in this PR:

  • Removed FILESYSTEM, the link is not necessary with GCC >= 9 which is our minimum.
  • Replaced FOLLYBENCHMARK with the proper target
  • Explicitly linked the gpu targets to CUDA
  • Improved bundled folly's use of glog and gflags (when also bundled)
  • Cleaned up the usage of an addition proto wrapper target so we don't link generated proto files twice (which doesn't cause issues in a static build)
  • Added a marker in the log so it's clear when we are running velox cmake vs. dependency cmake:
-- [xsimd] xsimd v10.0.0
-- [stemmer] Building stemmer from source
-- Setting Arrow source to AUTO
-- [Arrow] Checking for module 'thrift'

Initially I looked at forcing a bundled build of folly and gflags to make sure they are shared but that introduces weird rpath issues when the dependencies are also on the system (like our CI image), instead I just changed the build script to build folly as shared in the image, we maybe want to make that an option so people can keep folly as static (though even for static build shared folly will likely make the binary size much smaller.). If folly is build from source it will be built static or shared matching the velox build.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 13, 2024
Copy link

netlify bot commented Aug 13, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit a6afca1
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66db1e46f7e82a000825c63d

@assignUser assignUser mentioned this pull request Aug 13, 2024
@PHILO-HE
Copy link
Contributor

@assignUser, this is just a reply to the mentioned issue in PR description. In Gluten, we are using static folly lib, as well as gflags, for velox to link. Can upstream velox link these static libs also?

@assignUser
Copy link
Collaborator Author

@PHILO-HE that's what we normaly do as well but when building velox as an .so it doesn't work because of the mentioned linking issue.
gflags(static -> linked into folly(static) -> linked intod velox(shared) -> linked to tests(shared) -> test also links to gflags (static)

@PHILO-HE
Copy link
Contributor

@PHILO-HE that's what we normaly do as well but when building velox as an .so it doesn't work because of the mentioned linking issue. gflags(static -> linked into folly(static) -> linked intod velox(shared) -> linked to tests(shared) -> test also links to gflags (static)

@assignUser, I see. Thanks for your clarification!
Can we use link option --version-script to hide gflags symbols in the linking? We did this similarly in Gluten, which is a bit tricky maybe. https://github.com/apache/incubator-gluten/blob/b7918a9a4b3482c03283dc542a909669d2ef4534/cpp/core/symbols.map#L10

@assignUser assignUser force-pushed the mono-shared branch 2 times, most recently from f0b2964 to 654ad37 Compare August 22, 2024 15:58
@assignUser assignUser marked this pull request as ready for review August 22, 2024 22:47
@assignUser
Copy link
Collaborator Author

I am unclear on the one failed test, would be great if someone could have a look. But otherwise this is pretty done, we are currently using a temp CI image of course.

@assignUser assignUser mentioned this pull request Aug 22, 2024
Copy link
Collaborator

@czentgr czentgr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this!

scripts/setup-helper-functions.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@majetideepak majetideepak left a comment

Choose a reason for hiding this comment

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

@assignUser Thanks for adding this!

@DanielHunte
Copy link

I tried doing the monolithic shared build and ran into the error: Undefined symbols for architecture arm64. It seems to have something to do with facebook::velox::fuzzer::ArgumentTypeFuzzer::randOrderableType()?

[2584/3469] Linking CXX shared library lib/libvelox.dylib
ld: warning: ignoring duplicate libraries: 'CMake/resolve_dependency_modules/arrow/arrow_ep/install/lib/libarrow.a', 'CMake/resolve_dependency_modules/arrow/arrow_ep/src/arrow_ep-build/thrift_ep-install/lib/libthrift.a', '_deps/duckdb-build/src/libduckdb_static.a', '_deps/fmt-build/libfmtd.a', '_deps/protobuf-build/libprotobufd.a', '_deps/re2-build/libre2.a', '_deps/simdjson-build/libsimdjson.a'
[2586/3469] Linking CXX shared library velox/vector/tests/utils/libvelox_vector_test_lib.dylib
ld: warning: ignoring duplicate libraries: '/opt/homebrew/lib/libgtest.a'
[2595/3469] Linking CXX shared library velox/dwio/common/tests/utils/libvelox_dwio_common_test_utils.dylib
ld: warning: ignoring duplicate libraries: '/opt/homebrew/lib/libgtest.a', '_deps/fmt-build/libfmtd.a'
[2611/3469] Linking CXX shared library velox/expression/fuzzer/libvelox_expression_test_utility.dylib
FAILED: velox/expression/fuzzer/libvelox_expression_test_utility.dylib
: && /Library/Developer/CommandLineTools/usr/bin/c++ -mcpu=apple-m1+crc -D USE_VELOX_COMMON_BASE -D HAS_UNCAUGHT_EXCEPTIONS -Wall -Wextra -Wno-unused        -Wno-unused-parameter        -Wno-sign-compare        -Wno-ignored-qualifiers        -Wno-range-loop-analysis          -Wno-mismatched-tags          -Wno-nullability-completeness  -g -arch arm64 -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk -mmacosx-version-min=14.6 -dynamiclib -Wl,-headerpad_max_install_names -Wl,-flat_namespace -o velox/expression/fuzzer/libvelox_expression_test_utility.dylib -install_name @rpath/libvelox_expression_test_utility.dylib velox/expression/fuzzer/CMakeFiles/velox_expression_test_utility.dir/ArgumentTypeFuzzer.cpp.o velox/expression/fuzzer/CMakeFiles/velox_expression_test_utility.dir/FuzzerToolkit.cpp.o  -Wl,-rpath,/Users/danielhunte/velox/_build/debug/lib -Wl,-rpath,/Users/danielhunte/velox/_build/debug/_deps/folly-build -Wl,-rpath,/opt/homebrew/lib  lib/libvelox.dylib  /opt/homebrew/lib/libgtest.a  _deps/folly-build/libfolly.0.58.0-dev.dylib  /opt/homebrew/lib/libboost_context-mt.dylib  /opt/homebrew/lib/libboost_filesystem-mt.dylib  /opt/homebrew/lib/libboost_atomic-mt.dylib  /opt/homebrew/lib/libboost_program_options-mt.dylib  /opt/homebrew/lib/libboost_system-mt.dylib  /opt/homebrew/lib/libboost_thread-mt.dylib  /opt/homebrew/lib/libdouble-conversion.dylib  /opt/homebrew/lib/libgflags.2.2.2.dylib  /opt/homebrew/lib/libglog.dylib  /opt/homebrew/lib/libevent.dylib  /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/lib/libz.tbd  /opt/homebrew/Cellar/openssl@3/3.3.2/lib/libssl.dylib  /opt/homebrew/Cellar/openssl@3/3.3.2/lib/libcrypto.dylib  /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/lib/libbz2.tbd  /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/lib/liblzma.tbd  /opt/homebrew/lib/liblz4.dylib  /opt/homebrew/lib/libzstd.dylib  /opt/homebrew/lib/libsnappy.dylib  /opt/homebrew/lib/libsodium.dylib  -lc++abi  _deps/fmt-build/libfmtd.a  /opt/homebrew/lib/libgflags.2.2.2.dylib  /opt/homebrew/lib/libglog.dylib  _deps/re2-build/libre2.a  /opt/homebrew/lib/libboost_regex-mt.dylib  /opt/homebrew/lib/libdouble-conversion.3.3.0.dylib  _deps/protobuf-build/libprotobufd.a  /opt/homebrew/lib/libsnappy.dylib  /opt/homebrew/lib/libzstd.dylib  /opt/homebrew/lib/liblz4.dylib  /opt/homebrew/lib/liblzo2.dylib  /Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/lib/libz.tbd  CMake/resolve_dependency_modules/arrow/arrow_ep/install/lib/libarrow.a  CMake/resolve_dependency_modules/arrow/arrow_ep/src/arrow_ep-build/thrift_ep-install/lib/libthrift.a  velox/tpch/gen/dbgen/libdbgen.a  _deps/simdjson-build/libsimdjson.a  _deps/libstemmer/src/libstemmer/libstemmer.a  _deps/duckdb-build/src/libduckdb_static.a  _deps/duckdb-build/third_party/fsst/libduckdb_fsst.a  _deps/duckdb-build/third_party/fmt/libduckdb_fmt.a  _deps/duckdb-build/third_party/libpg_query/libduckdb_pg_query.a  _deps/duckdb-build/third_party/re2/libduckdb_re2.a  _deps/duckdb-build/third_party/miniz/libduckdb_miniz.a  _deps/duckdb-build/third_party/utf8proc/libduckdb_utf8proc.a  _deps/duckdb-build/third_party/hyperloglog/libduckdb_hyperloglog.a  _deps/duckdb-build/third_party/fastpforlib/libduckdb_fastpforlib.a  _deps/duckdb-build/third_party/mbedtls/libduckdb_mbedtls.a && :
Undefined symbols for architecture arm64:
  "facebook::velox::randOrderableType(std::__1::mersenne_twister_engine<unsigned int, 32ul, 624ul, 397ul, 31ul, 2567483615u, 11ul, 4294967295u, 7ul, 2636928640u, 15ul, 4022730752u, 18ul, 1812433253u>&, int)", referenced from:
      facebook::velox::fuzzer::ArgumentTypeFuzzer::randOrderableType() in ArgumentTypeFuzzer.cpp.o
  "facebook::velox::randType(std::__1::mersenne_twister_engine<unsigned int, 32ul, 624ul, 397ul, 31ul, 2567483615u, 11ul, 4294967295u, 7ul, 2636928640u, 15ul, 4022730752u, 18ul, 1812433253u>&, int)", referenced from:
      facebook::velox::fuzzer::ArgumentTypeFuzzer::randType() in ArgumentTypeFuzzer.cpp.o
ld: symbol(s) not found for architecture arm64
c++: error: linker command failed with exit code 1 (use -v to see invocation)
[2620/3469] Building CXX object velox/expression/fuzzer/CMakeFiles/velox_expression_fuzzer.dir/ExpressionFuzzer.cpp.o
In file included from /Users/danielhunte/velox/velox/expression/fuzzer/ExpressionFuzzer.cpp:28:
In file included from /Users/danielhunte/velox/./velox/expression/SimpleFunctionRegistry.h:21:
/Users/danielhunte/velox/./velox/expression/SimpleFunctionAdapter.h:286:62: warning: 'unique' is deprecated [-Wdeprecated-declarations]
  286 |       if (args[POSITION]->isFlatEncoding() && args[POSITION].unique() &&
      |                                                              ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__memory/shared_ptr.h:730:3: note: 'unique' has been explicitly marked deprecated here
  730 |   _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_HIDE_FROM_ABI bool unique() const _NOEXCEPT { return use_count() == 1; }
      |   ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__config:1022:41: note: expanded from macro '_LIBCPP_DEPRECATED_IN_CXX17'
 1022 | #    define _LIBCPP_DEPRECATED_IN_CXX17 _LIBCPP_DEPRECATED
      |                                         ^
/Library/Developer/CommandLineTools/SDKs/MacOSX15.0.sdk/usr/include/c++/v1/__config:995:49: note: expanded from macro '_LIBCPP_DEPRECATED'
  995 | #      define _LIBCPP_DEPRECATED __attribute__((__deprecated__))
      |                                                 ^
1 warning generated.
ninja: build stopped: subcommand failed.
make[1]: *** [build] Error 1
make: *** [debug] Error 2```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants