Build and link libfolly with RocksDB #10103

anand1976 · 2022-06-02T23:36:35Z

The current integration with folly requires cherry-picking folly source files to include in RocksDB for external CI builds. Its not scaleable as we depend on more features in folly, such as coroutines. This PR adds a dependency from RocksDB to the folly library when USE_FOLLY or USE_COROUTINES are set. We build folly using the build scripts in third-party/folly, relying on it to download and build its dependencies. A new Makefile target, build_folly, is provided to make building folly easier.

A new option, USE_FOLLY_LITE is added to retain the old model of compiling selected folly sources with RocksDB. This might be useful for short-term development.

mdcallag · 2022-07-19T22:48:48Z

What version of gcc does folly require? I use Ubuntu 20.04 and 22.04 -- both are LTS (long term supported) but 22.04 has yet to be fully released. With 20.04 the default gcc is 9.4 and with 22.04 it is 11.2.

pdillinger

Also, can we get some build size numbers? That might influence non-fbcode users.

pdillinger · 2022-07-19T23:14:21Z

Makefile

 	@# NOTE: this hack is required for clang in some cases
 	perl -pi -e 's/int rv = syscall/int rv = (int)syscall/' third-party/folly/folly/detail/Futex.cpp
 	@# NOTE: this hack is required for gcc in some cases
 	perl -pi -e 's/(__has_include.<experimental.memory_resource>.)/__cpp_rtti && $$1/' third-party/folly/folly/memory/MemoryResource.h

+build_folly:


Update: Mark changed my perspective in later comments.

Original feedback:

In my experience, quality builds are divided into (up to) two distinct phases:

download any dependencies (without running the compiler)

run the compiler and maybe tests (without hitting the network)

And there should be a "clean" step that gets you back to the state after 1 so that you can rebuild with a different compiler or compilation settings. Ideally, each of those two phases requires only one command invocation.

What that means here (I think):

We should download, etc. any/all the folly dependencies as part of the checkout_folly step.

If configured to use folly, we should automatically build libfolly.a if it doesn't exist as part of normal "make check" or "make all". We don't need to do dependency tracking in folly, because we are not intending to develop on it from rocksdb repo.

If third-party/folly exists, then clean folly build outputs as part of our "make clean", so that libfolly.a is rebuilt automatically if changing compilers, etc.

pdillinger · 2022-07-19T23:20:48Z

Makefile

+		echo "Please run checkout_folly first"; \
+		false; \
+	fi
+	cd third-party/folly && CXXFLAGS=" -mavx2 -DHAVE_CXX11_ATOMIC " $(PYTHON) build/fbcode_builder/getdeps.py build --no-tests


Hmm, hard-coded dependency on x86 (avx2). Perhaps you can do something like

&& MAYBE_AVX2=`echo $(CXXFLAGS) | grep -o -- -mavx2 || true` && CXXFLAGS=" $$MAYBE_AVX2 -DHAVE_CXX11_ATOMIC " ...

anand1976 · 2022-07-20T14:27:01Z

What version of gcc does folly require? I use Ubuntu 20.04 and 22.04 -- both are LTS (long term supported) but 22.04 has yet to be fully released. With 20.04 the default gcc is 9.4 and with 22.04 it is 11.2.

@mdcallag Folly coroutine support requires gcc 10 or newer. But if you're not using coroutines, gcc 7+ would work.

mrambacher · 2022-07-20T16:19:19Z

CMakeLists.txt

+  add_compile_definitions(USE_FOLLY FOLLY_NO_CONFIG HAVE_CXX11_ATOMIC)
+  list(APPEND THIRDPARTY_LIBS Folly::folly)
+  set(FOLLY_LIBS Folly::folly)
+  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,--copy-dt-needed-entries")
 endif()


Why not use find_package instead?

It did not work when the folly cmake dir was specified in the CMAKE_PREFIX_PATH. I can add calls to find_package, with a fallback to this approach if find_package fails. That'll at least allow an existing folly installation to be specified.

mrambacher · 2022-07-20T16:20:13Z

build_tools/build_detect_platform

+# Get the path for the folly installation dir
+if [ -z "$FOLLY_DIR" ]; then
+  FOLLY_DIR="./third-party/folly"
+fi
+FOLLY_PATH=`cd $FOLLY_DIR && $PYTHON build/fbcode_builder/getdeps.py show-inst-dir folly`
+


Why not detect folly like any other add-on (such as ZSTD)?

Folly doesn't have proper versions with compatibility promises (see Folly RPM for CentOS 7 and CentOS 6 folly#929 (comment))

Folly uses a lot of conditional compilation and implementation-in-header, which likely affects linkability even between ABI-compatible compilers (because ODR, etc.)

Folly tends to track newer C++ standards to take advantage of features

As a result of these things, pre-packaged folly binary+headers is not really a thing, and not expected to be in the foreseeable future.

This is not pre-packaging. This is "if I already have folly installed and am using it in one tool, why can I not use that same code I built?

CacheLib uses folly. If there is a secondary cache that uses CacheLib, then RocksDB will end up with two versions of folly linked into it. Why can't all of the tools use the same version of folly?

OK, you've convinced me that we should think about this as an optional external dependency, potentially with a shortcut script to checkout & build a known-compatible version.

By the way our internal solution to this problem is "use buck."

Makes sense. Maybe, if FOLLY_DIR is not specified, we don't try to detect and just assume the user has set the CXXFLAGS and EXEC_LDFLAGS environment variables appropriately.

pdillinger · 2022-07-21T15:05:03Z

Makefile

+	# For some reason, glog and fmt libraries are under either lib or lib64
+	GLOG_LIB_PATH = $(shell (ls -d $(GLOG_PATH)/lib*))
+	FMT_LIB_PATH = $(shell (ls -d $(FMT_PATH)/lib*))
+
 	# AIX: pre-defined system headers are surrounded by an extern "C" block
 	ifeq ($(PLATFORM), OS_AIX)
 		PLATFORM_CCFLAGS += -I$(FOLLY_DIR)


I think these two lines should be removed, like below

pdillinger · 2022-07-21T15:14:45Z

Makefile

@@ -451,18 +456,37 @@ ifeq ($(USE_FOLLY),1)
 	ifeq (,$(FOLLY_DIR))


Now that we have FOLLY_PATH, FOLLY_DIR is confusing. Perhaps call it FOLLY_SRC. It would be nice if FOLLY_SRC were optional (if libfolly.a and headers already available in FOLLY_PATH).

pdillinger · 2022-07-21T15:25:06Z

.circleci/config.yml

+    steps:
+      - pre-steps
+      - run: sudo apt-get update -y && sudo apt-get install gcc-10 g++-10 libgflags-dev
+      - setup-folly


Under the paradigm of folly being an external library, it would be nice if one shared CircleCI step downloaded & compiled folly, though we would probably need to add CC and CXX to the environment for this build to make that work (which would reduce copy-paste anyway).

pdillinger · 2022-07-21T15:27:30Z

CMakeLists.txt

+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fcoroutines -Wno-maybe-uninitialized")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-deprecated")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-redundant-move")
+  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-invalid-memory-model")


Are these suppressed warnings for stuff in our code, or related to system / third party headers?

These are for warnings in the third-party code.

pdillinger · 2022-07-21T15:27:54Z

CMakeLists.txt

 if(USE_FOLLY)
-  include_directories(${PROJECT_SOURCE_DIR}/third-party/folly)


(I don't know cmake well enough to advise here.)

pdillinger · 2022-07-21T15:32:21Z

Makefile

@@ -549,10 +573,6 @@ LIB_OBJECTS += $(patsubst %.c, $(OBJ_DIR)/%.o, $(LIB_SOURCES_C))
 LIB_OBJECTS += $(patsubst %.S, $(OBJ_DIR)/%.o, $(LIB_SOURCES_ASM))
 endif

-ifeq ($(USE_FOLLY),1)
-  LIB_OBJECTS += $(patsubst %.cpp, $(OBJ_DIR)/%.o, $(FOLLY_SOURCES))


Should remove FOLLY_SOURCES from src.mk

facebook-github-bot · 2022-08-04T15:05:25Z

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

pdillinger

Overall LGTM

pdillinger · 2022-08-22T23:33:31Z

Makefile

@@ -139,9 +139,13 @@ endif
 GIT_COMMAND ?= git
 ifeq ($(USE_COROUTINES), 1)
 	USE_FOLLY = 1
-	OPT += -DUSE_COROUTINES
+	OPT += -DUSE_COROUTINES -DHAVE_CXX11_ATOMIC


Isn't HAVE_CXX11_ATOMIC unused / obsolete?

Its used in glog/logging.h. I guess glog still supports very old compilers.

pdillinger · 2022-08-22T23:44:14Z

Makefile

@@ -449,9 +458,43 @@ endif
 # This provides a Makefile simulation of a Meta-internal folly integration.
 # It is not validated for general use.
 ifeq ($(USE_FOLLY),1)


I'd like to see some kind of comment explaining USE_FOLLY vs. USE_FOLLY_LITE and FOLLY_DIR vs. FOLLY_PATH.

pdillinger · 2022-08-22T23:53:57Z

.circleci/config.yml

+      - pre-steps
+      - run: sudo apt-get update -y && sudo apt-get install gcc-10 g++-10 libgflags-dev
+      - setup-folly
+      - build-folly


I guess in other cases the system compiler is ABI compatible with the compiler used with RocksDB, but not in this case? (I would still expect ODR violations between compilers given how much conditional compilation is in folly headers.)

In this case, we need g++ 10 for coroutine support.

pdillinger · 2022-08-22T23:55:18Z

.circleci/config.yml

+      - run: sudo apt-get update -y && sudo apt-get install gcc-10 g++-10 libgflags-dev
+      - setup-folly
+      - build-folly
+      - run: (mkdir build && cd build && CC=gcc-10 CXX=g++-10 cmake -DUSE_COROUTINES=1 -DWITH_GFLAGS=1 -DROCKSDB_BUILD_SHARED=0 .. && CC=gcc-10 CXX=g++-10 make V=1 -j20 && ctest -j20)


Don't need to set CC and CXX again if in environment.

facebook-github-bot · 2022-09-01T22:33:53Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-01T22:35:59Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-02T01:44:57Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-06T18:55:10Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-06T19:00:13Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-09T00:06:07Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-09T00:38:05Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

Summary: Make folly an external dependency by trying to detect it in build_detect_platform

facebook-github-bot · 2022-09-09T16:30:57Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-09T18:22:30Z

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

facebook-github-bot · 2022-09-09T19:25:06Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-09T19:25:32Z

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

facebook-github-bot · 2022-09-12T02:14:58Z

@anand1976 has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-09-12T02:15:42Z

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

facebook-github-bot added the CLA Signed label Jun 2, 2022

anand1976 mentioned this pull request Jun 17, 2022

Use optimized folly DistributedMutex in LRUCache when available #10179

Closed

anand1976 force-pushed the build_with_folly branch 2 times, most recently from 17170da to 1acc8fa Compare June 30, 2022 21:13

anand1976 changed the title ~~cmake build with folly coroutines~~ Add dependency on libfolly.a Jun 30, 2022

anand1976 marked this pull request as ready for review June 30, 2022 23:56

anand1976 requested review from pdillinger and akankshamahajan15 June 30, 2022 23:56

pdillinger reviewed Jul 19, 2022

View reviewed changes

mrambacher reviewed Jul 20, 2022

View reviewed changes

pdillinger reviewed Jul 21, 2022

View reviewed changes

anand1976 force-pushed the build_with_folly branch from 12d29f6 to 12179ba Compare August 1, 2022 17:00

anand1976 requested a review from pdillinger August 4, 2022 19:33

anand1976 changed the title ~~Add dependency on libfolly.a~~ Build and link libfolly with RocksDB Aug 4, 2022

pdillinger approved these changes Aug 22, 2022

View reviewed changes

anand1976 force-pushed the build_with_folly branch from a0acafe to ca369fc Compare September 1, 2022 22:33

anand76 added 4 commits September 9, 2022 09:30

Build and link with folly library in third-party/folly

d7b5979

Fix order of searching and linking libraries

38340b9

Change library order

fec1648

Address some more comments

cbf88e2

Summary: Make folly an external dependency by trying to detect it in build_detect_platform

anand76 added 11 commits September 9, 2022 09:30

Fixes

aec351a

Address more comments; Introduce USE_FOLLY_LITE

03f206f

Fix avx2 flag

ed570c0

Add folly lite tests to CircleCI

5cbebb9

Fix gcc-7 folly lite test in CircleCI

04f7ae8

Address comments

44fc750

Remove upgrade-cmake from build-linux-cmake-with-folly-lite-no-test

6d46842

Try to fix CircleCI folly tests to run with docker image

84c119d

Fix CircleCI

8ad8249

Update Docker image with OpenSSL

db3b1ba

Use Docker image for folly builds

f45c2a3

anand1976 force-pushed the build_with_folly branch from 44037b2 to f45c2a3 Compare September 9, 2022 16:30

Replace tabs with spaces

8edb1a1

Replace tab with spaces

c4cb45f

facebook-github-bot closed this in be09943 Sep 12, 2022

		@@ -451,18 +456,37 @@ ifeq ($(USE_FOLLY),1)
		ifeq (,$(FOLLY_DIR))

		if(USE_FOLLY)
		include_directories(${PROJECT_SOURCE_DIR}/third-party/folly)

Build and link libfolly with RocksDB #10103

Build and link libfolly with RocksDB #10103

Conversation

anand1976 commented Jun 2, 2022 • edited Loading

mdcallag commented Jul 19, 2022

pdillinger left a comment

Choose a reason for hiding this comment

pdillinger Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anand1976 commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anand1976 Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 4, 2022

pdillinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Sep 1, 2022

facebook-github-bot commented Sep 1, 2022

facebook-github-bot commented Sep 2, 2022

facebook-github-bot commented Sep 6, 2022

facebook-github-bot commented Sep 6, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 9, 2022

facebook-github-bot commented Sep 12, 2022

facebook-github-bot commented Sep 12, 2022

anand1976 commented Jun 2, 2022 •

edited

Loading

pdillinger Jul 19, 2022 •

edited

Loading

anand1976 Jul 28, 2022 •

edited

Loading