Skip to content
This repository was archived by the owner on May 9, 2024. It is now read-only.

Enable folly #293

Merged
merged 8 commits into from
Mar 21, 2023
Merged

Enable folly #293

merged 8 commits into from
Mar 21, 2023

Conversation

alexbaden
Copy link
Contributor

This PR is almost identical to #226, but includes one additional commit which propagates the folly compiler definition to the cython code. The previous segfault occurred because cython code was using a std::shared_timed_mutex, whereas the compiled libraries were using folly::mutex.

This does bring up an interesting issue which is the need to propagate compiler definitions to cython more generally. I just did a one-off to get folly working, but I am interested in ways to propagate the set definitions using cmake variables (as a followup).

Also, I found a cython flag to enable gdb. On ubuntu using the conda environment, this gave me enough debug symbols (with regular gdb) to get a stacktrace. I turned the flag off but left it in setup.py.in for future use.

Supersedes #226

@alexbaden alexbaden force-pushed the alex_pakurapo/folly branch 2 times, most recently from 2243e7a to 0f5b6d6 Compare March 18, 2023 23:11
@alexbaden alexbaden marked this pull request as draft March 20, 2023 17:39
@alexbaden alexbaden marked this pull request as ready for review March 20, 2023 22:14
@alexbaden
Copy link
Contributor Author

All issues have been resolved --

  1. ENABLE_FOLLY was not being propagated to python code. This resulted in a different mutex type being used in python (std) vs compiled library (folly).
  2. The ResultSetTest was using too much memory. This is specifically the large buffers reduction tests, which create a 2^32 size buffer to mimic overflow of a 32-bit group by. This test runs fine on self hosted docker, but does not run on ghrunners, so I disabled it on ghrunners. I was also able to remove the special memory conditions for sanity test (previously smoke test), which do not work for folly because we end up making more use of virtual memory with the Arenas.
  3. Modern cmake targets were used in some places, but this resulted in Folly_LIBRARIES being empty in cmake. I use modern cmake targets now and push them back into Folly_LIBRARIES, since other libraries get packed w/ folly (particularly on Windows).

There was one failure in NoCatalogSqlTest but that is a threading issue, and appears unrelated to folly, so this is now ready for review.

I would like to handle the merge as I want to squash everything into one commit and re-word.

@ienkovich
Copy link
Contributor

The previous segfault occurred because cython code was using a std::shared_timed_mutex, whereas the compiled libraries were using folly::mutex.

This is a good catch. We also have other similar conditional declarations, e.g. for threading. We should get all definitions from COMPILE_DEFINITIONS property to be more safe here. I created #297 for that.

Copy link
Contributor

@kurapov-peter kurapov-peter left a comment

Choose a reason for hiding this comment

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

Thanks! There're some minor cmake comments

# TODO: use Folly::folly_deps?
if(MSVC)
find_package(Libevent COMPONENTS core REQUIRED)
list(APPEND Folly_LIBRARIES libevent::core)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Folly::folly already exports it, so you don't need it

@@ -49,6 +49,10 @@ if (NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)
"Debug" "Release" "MinSizeRel" "RelWithDebInfo")
endif ()

# External Dependencies
list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake_modules")
list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/omniscidb/cmake/Modules")
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish we could avoid using any legacy cmake stuff

@alexbaden alexbaden merged commit ca2ccf7 into main Mar 21, 2023
@alexbaden alexbaden deleted the alex_pakurapo/folly branch March 21, 2023 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants