velox icon indicating copy to clipboard operation
velox copied to clipboard

Add option to build monolithic, shared library

Open assignUser opened this issue 1 year ago • 6 comments

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.

assignUser avatar Aug 13 '24 04:08 assignUser

Deploy Preview for meta-velox canceled.

Name Link
Latest commit 5920c93f3b0aa9821b8178d8e71611a7ea5c1984
Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/675310f4f6e297000847d3f4

netlify[bot] avatar Aug 13 '24 04:08 netlify[bot]

@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?

PHILO-HE avatar Aug 19 '24 02:08 PHILO-HE

@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 avatar Aug 19 '24 03:08 assignUser

@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

PHILO-HE avatar Aug 19 '24 03:08 PHILO-HE

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 avatar Aug 22 '24 23:08 assignUser

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```

DanielHunte avatar Oct 07 '24 18:10 DanielHunte

Importing to check internal build.

kgpai avatar Nov 26 '24 21:11 kgpai

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 26 '24 21:11 facebook-github-bot

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Dec 06 '24 17:12 facebook-github-bot

@kgpai, @assignUser Should we wait until https://github.com/facebookincubator/velox/pull/11745 lands?

majetideepak avatar Dec 07 '24 11:12 majetideepak

@kgpai merged this pull request in facebookincubator/velox@164b4c2beb8ec926fdb1e270c24aa6d38ccb195a.

facebook-github-bot avatar Dec 08 '24 01:12 facebook-github-bot