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.

Sign up for GitHub

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

Fix and improve jemalloc releated cmake #52

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

Fix and improve jemalloc releated cmake #52

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c2a557e
Fix JEMALLOC_PREFIX when `--with-jemalloc-prefix=` is absent.
ur4t Apr 19, 2024
c27a00b
Use `pkg-config` to find `jemalloc` if available.
ur4t Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@ set(DEP_LIBS_PKG_ARG_LISTS "IpcShm ${DEP_IPC_SHM_VERSION} CONFIG")
# (TODO: but look into the jemalloc/jemalloc-cmake GitHub repo -- what's that all about?)
# Hence we must ensure 2 things more manually: the lib location and the include-path.

# Try pkg-config if available
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few related things going on possibly outside the issues you were trying to (and succeeding to) fix/improve, so I might make a new PR after looking into everything. I omit details here to avoid extra confusion. Anyway I shall look into that soon but probably not right this second.

Even before then, let me put some comments/questions here, before I forget. Your answers (if any) will serve to at least educate me. (Undoubtedly I am a CMake noob; in fact I learned it, to the extent that I did, for this project.)

--

  • This looks delightful -- but shouldn't this be within if(NOT JEMALLOC_LIB)? To the extent that this is better than find_library() and all that, perhaps it should be attempted a bit below, and if it fails then go on to my find_library() stuff?
  • I am also curious; and again this is most likely just for my own education; what moved you to make this change? Was it a general desire to improve this, or did you observe (or know of a use case where one would observe) the find_library() stuff failing, where pkg-config would have succeeded? If it's only a general desire to improve this, that doesn't mean we shouldn't; I just want to know the reason is all.

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't this be within if(NOT JEMALLOC_LIB)?

Agree. This shoubld be within if(NOT JEMALLOC_LIB).

Personally I perfer the following logic:

  1. Use user specified elements (but via other variable names, explanation later).
  2. If available, use find_package() to find corresponding elements.
  3. Else, use find_package(PkgConfig) and the following pkg_check_modules() (which drives pkgconf or old pkg-config) to find corresponding elements.
  4. (Not recommended) manually call find_library() and find_path() and set corresponding elements.

Usually find_package() is the approach recommended by CMake, but regrettably jemalloc does not provided files used by find_package(). jemalloc.pc is provided and much more cleaner than using find_library() and find_path() directly.

pkg_check_modules(JEMALLOC REQUIRED jemalloc)
if(JEMALLOC_FOUND)
set(JEMALLOC_LIB "${JEMALLOC_LIBRARIES}")
set(JEMALLOC_INCLUDE_PATH "${JEMALLOC_INCLUDEDIR}")
Comment on lines +60 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these lines set them as CACHE variables? I ask, because find_library() and find_path() do so (unless one sets NO_CACHE, which I did not).

Copy link
Author

Choose a reason for hiding this comment

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

PS: Actually jemalloc releated cmake can be further simplified, current modifications are minimized to keep consistent with the existing approach.

I am not sure, they are just aliases to avoid more modifications.

Personally I recommend use JEMALLOC_LIBRARIES and JEMALLOC_INCLUDEDIR directly rather than shimming via JEMALLOC_LIB and JEMALLOC_INCLUDE_PATH:

https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules says that <XXX>_LIBRARIES and <XXX>_INCLUDE_DIRS are set by pkg_check_modules(). I am not quite sure whether find_package() use these names too or not, but as far as I remember, manually specifying other libraries usually requires -D <LIBNAME>_LIBRARIES=PATH_TO_LIB_DIR.

unset(JEMALLOC_PREFIX CACHE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? If they specified this prefix manually for some unholy reason (and we do advertise this), maybe we should let them? (I have the vague feeling CMake's philosophy on cache variables isn't totally clear... at least I couldn't glean it when I tried... like this value could be a wrongly-computed value from a previous run, in which case who knows what we should do - but presumably worst-case user can just clear cache and start over....)

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant just the unset() line there... not the 3 preceding that. Apologies for being confusing.

Copy link
Author

Choose a reason for hiding this comment

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

https://cmake.org/cmake/help/latest/module/FindPkgConfig.html#command:pkg_check_modules says that a <YYY>_PREFIX is set by pkg_check_modules(), and I was trying to avoid potential conflict. (I mistakenly thought JEMALLOC_PREFIX is only set by the following jemalloc-config utilities and not supposed to be specified by users.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, got it. There's quite a bit going on. I can surely improve this based on stuff you've taught me (plus subsequent experiments). Like I don't need to introduce more cache variables if some would already be set with the pkg... stuff... and clearly I should differentiate in naming between prefix as in path prefix, versus prefix as in the API name prefix (which is super duper important here). And probably more boring subtleties like that.

The time to fix it up is now, before (god willing) all kinds of users start using mr Flow-IPC.

So, not ready to go yet, but I'm optimistic. 👍🏻

Copy link
Author

Choose a reason for hiding this comment

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

A different name like JEMALLOC_API_PREFIX is inconsistent with upstream jemalloc's convention, which might be confusing.

I think some "save/restore" actions might help here, with proper explaining comments: Save user specified JEMALLOC_PREFIX to another variable temporarily and restore JEMALLOC_PREFIX with that saver variable after pkg_check_modules() stuffs.

Be bold and do it! Changes are never too late. 👍🏻👍🏻👍🏻

endif()

# Firstly: Finding the (static) library. This approach is recommended by FlowLikeLib.cmake docs.
if(NOT JEMALLOC_LIB)
block()
Expand Down Expand Up @@ -131,9 +139,10 @@ function(jemalloc_config_run)
OUTPUT_VARIABLE output
OUTPUT_STRIP_TRAILING_WHITESPACE
COMMAND_ERROR_IS_FATAL ANY)
string(REGEX REPLACE ".*--with-jemalloc-prefix=([^ ]*).*" "\\1" JEMALLOC_PREFIX ${output})
if(${JEMALLOC_PREFIX} STREQUAL "")
set(${JEMALLOC_PREFIX} "${NONE}")
string(REGEX MATCH "--with-jemalloc-prefix=[^ ]*" JEMALLOC_PREFIX "${output}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this... it seems like clearly an improvement/defensive in nature, at the very least.

Again for my own education I ask how you came across the desire to do this. Is it that if one builds jemalloc without explicitly setting this prefix thing to blank when configuring it, then the jemalloc-config output will omit --with-jemalloc-prefix= entirely?

(That may or may not be related to a certain problem someone found when setting the prefix to . But don't worry about that... my thing to track down. Bottom line is you may have fixed here the problem they found-- I am just not sure yet.)

Copy link
Author

Choose a reason for hiding this comment

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

Is it that if one builds jemalloc without explicitly setting this prefix thing to blank when configuring it, then the jemalloc-config output will omit --with-jemalloc-prefix= entirely?

Yes!

Distros using RPM (Fedora and OpenSUSE) ship jemalloc-devel with jemalloc-config script, whose output contains no --with-jemalloc-prefix.

Distros using DEB (Debian and Ubuntu) ship libjemalloc-dev without jemalloc-config script at all.

On Debian, I built jemalloc myself (without setting any configuration options) to provided jemalloc-config and jemalloc-config --config outputs a blank line.

Actually jemalloc-config --config outputs arguments provided to ./configure ... as is. And usually the JEMALLOC_PERFIX is not set to utilize LD_PRELOAD functionality.

string(REPLACE "--with-jemalloc-prefix=" "" JEMALLOC_PREFIX "${JEMALLOC_PREFIX}")
if("${JEMALLOC_PREFIX}" STREQUAL "")
set(JEMALLOC_PREFIX "${NONE}")
endif()

# "Fun" quirk of CMake: a cache setting can't be straightforwardly modified from within a function.
Expand Down