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

Feature request: cmake support #303

Open
namreeb opened this issue Nov 21, 2015 · 61 comments
Open

Feature request: cmake support #303

namreeb opened this issue Nov 21, 2015 · 61 comments
Assignees
Labels
Milestone

Comments

@namreeb
Copy link

@namreeb namreeb commented Nov 21, 2015

CMake support would be wonderful for cross-platform support. It would allow easy integration into existing cross-platform projects.

@jasone jasone added the portability label Jan 11, 2016
@rustyx

This comment has been minimized.

Copy link
Contributor

@rustyx rustyx commented Feb 26, 2016

+1. Not to mention CMake is much more BSD-friendly license-wise.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 26, 2016

I build internally with CMake but there are 3 Windows specific places which can be fixed/augumented. However, I do not have other platforms handy to make it gneric.

@jasone

This comment has been minimized.

Copy link
Member

@jasone jasone commented Feb 27, 2016

I'm open to switching over to CMake, but it's way down my list of things to do right now. If someone besides me is passionate about converting all the portability logic in configure.ac and making the new build system function reliably on at least Linux, FreeBSD, OS X, and Windows, I'm willing to do my part to learn more about CMake and help maintain it as the sole build system.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

Is there an overview of the different build environments jemalloc currently supports? I might be willing to undertake this.

@jasone

This comment has been minimized.

Copy link
Member

@jasone jasone commented Feb 29, 2016

@wbierman, most of the supported platforms are enumerated in configure.ac.

@photex

This comment has been minimized.

Copy link

@photex photex commented Feb 29, 2016

+1 I just started to work on this for Linux and OSX. :)

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 29, 2016

+1 I think a good idea would be to issue a fairly generic CMake and then a few people could take a platform each and fix it up. That would parallelize the effort and would not tax a specific individual trying to support everything that is in configure.ac, This is a high bar to get something out that might benefit people immediatly.

However, that would require a short transition period where CMake file would exist in parallel with the existing system. If @jasone would soften his stand on having 4 platform support at the first iteration, although those 4 may become available almost immediately due to the nature of CMake.

@jasone

This comment has been minimized.

Copy link
Member

@jasone jasone commented Feb 29, 2016

I'm fine with an iterative approach to CMake transition, but I don't want to do a release with both build systems. Incremental changes (and refactoring diffs that touch files outside the new CMake-related files) need to be self-contained so that we can cherry-pick around them for bugfix releases, and leave them out entirely if the effort isn't complete in time for the 4.2.0 release.

Re: 4.2.0 release date, pretty much all of the features we're currently considering are non-trivial, and it's likely to be at least a couple months before we have enough functionality to justify release.

@rustyx

This comment has been minimized.

Copy link
Contributor

@rustyx rustyx commented Feb 29, 2016

Sounds like a good candidate for a feature branch.

@jasone

This comment has been minimized.

Copy link
Member

@jasone jasone commented Feb 29, 2016

Good idea.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

I was going to suggest perhaps another repository under this organization so that those of who are interested in contributing might be able to do so more easily? @photex mentioned already having started on Linux and OSX. I can work on Windows and would be willing to setup virtual machines to work on the more obscure platforms if it helps to get the change adopted.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 29, 2016

@wbierman I can share my Windows CMake build as a starting point or just as a reference if there interest.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

Sounds good to me.

@jasone would you be willing to create a 'jemalloc-cmake' repository and add myself, @yuslepukhin, @rustyx, @photex to it?

@jasone

This comment has been minimized.

Copy link
Member

@jasone jasone commented Feb 29, 2016

Okay, https://github.com/jemalloc/jemalloc-cmake is set up. GitHub doesn't directly support forks within the same project, so I had to do the following to create the repo:

git clone git@github.com:jemalloc/jemalloc-cmake.git
git remote add -m dev upstream https://github.com/jemalloc/jemalloc.git
git pull upstream dev
git push origin master

It looks like you will manually need to add the remote to your cloned repo so that you can merge the upstream into the jemalloc-cmake repo master branch from time to time.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

I love git. It is so versatile.

@yuslepukhin if you would care to add your efforts to our new repository and ping me I will try and get started based off of that.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 29, 2016

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 29, 2016

@wbierman Need a label to work otherwise need a fix for version

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

@wbierman Shared via jemalloc/jemalloc-cmake@84c3dea

Great, thanks!

@wbierman Need a label to work otherwise need a fix for version

I don't understand this message.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Feb 29, 2016

@wbierman GitVersion parsing Utility function code fails if there are no tags on the repository. So I pushed a new tag which you may have pulled already. If so it will work. However, still need improvement in case a tag is missing.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Feb 29, 2016

@yuslepukhin Ah, okay. No problem. I haven't pulled it yet. Thanks.

@dibyendumajumdar

This comment has been minimized.

Copy link

@dibyendumajumdar dibyendumajumdar commented Mar 19, 2016

Hi - I was excited to see the CMake / Visual Studio 2015 build option - many thanks for working on this!
I am getting errors though when trying to create the project files:

CMake Error at CMakeLists.txt:393 (GeneratePublicSymbolsList):
GeneratePublicSymbolsList Function invoked with incorrect arguments for
function named: GeneratePublicSymbolsList
CMake Error at Utilities.cmake:467 (file):
file STRINGS file
"C:/github/jemalloc-cmake/include/jemalloc/internal/public_symbols.txt"
cannot be read.
Call Stack (most recent call first):
CMakeLists.txt:401 (GenerateJemallocRename)
CMake Error at CMakeLists.txt:404 (GenerateJemallocMangle):
GenerateJemallocMangle Function invoked with incorrect arguments for
function named: GenerateJemallocMangle
CMake Error at Utilities.cmake:427 (file):
file STRINGS file
"C:/github/jemalloc-cmake/include/jemalloc/internal/public_symbols.txt"
cannot be read.
Call Stack (most recent call first):
CMakeLists.txt:408 (GenerateJemallocMangle)
-- Creating public header C:/github/jemalloc-cmake/include/jemalloc/jemalloc.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc.h.header
C:\github\jemalloc-cmake\include\jemalloc\jemalloc_defs.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc_rename.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc_macros.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc_protos.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc_typedefs.h
C:\github\jemalloc-cmake\include\jemalloc\jemalloc.h.footer
1 file(s) copied.
CMake Error at Utilities.cmake:549 (file):
file STRINGS file
"C:/github/jemalloc-cmake/include/jemalloc/internal/public_symbols.txt"
cannot be read.
Call Stack (most recent call first):
CMakeLists.txt:427 (PublicNamespace)
CMake Error at Utilities.cmake:563 (file):
file STRINGS file
"C:/github/jemalloc-cmake/include/jemalloc/internal/public_symbols.txt"
cannot be read.
Call Stack (most recent call first):
CMakeLists.txt:430 (PublicUnnamespace)
-- Please wait while we configure class sizes
-- Finished configuring class sizes
-- Configuring incomplete, errors occurred!

@dibyendumajumdar

This comment has been minimized.

Copy link

@dibyendumajumdar dibyendumajumdar commented Mar 19, 2016

Hi, looks like the error occurs when I specify the option 'with-jemalloc-prefix'; removing this option led to successful build. But two tests failed - hash and tsd.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Mar 22, 2016

You need to specify a value for with-jemalloc-prefix. I have all tests pass in my runs. Said that, the CMake file has not been updated for latest changes in jemalloc and you need to use it out of jemalloc-cmake project where the effort is under way.

@dibyendumajumdar

This comment has been minimized.

Copy link

@dibyendumajumdar dibyendumajumdar commented Mar 24, 2016

Hi - I used the jemalloc-cmake project and also gave a value for with-jemalloc-prefix. Maybe I am not doing it right? Please could you post an example invocation?

I ran the tests under Windows 10 64-bit compiled using Visual Studio 2015. And as reported a couple of tests failed.

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Mar 25, 2016

@dibyendumajumdar Fixed the issue with jemalloc/jemalloc-cmake@7ceb9b3

Here is an example using Studio 12, just change to 14
c:\dev\oss\jemalloc-cmake\build>CMake -G "Visual Studio 12 Win64" -Ddisable-fill=1 -Dwith-malloc-conf=purge:decay -Dwith-jemalloc-prefix=cus_ ..
-- The C compiler identification is MSVC 18.0.40629.0
-- Check for working C compiler using: Visual Studio 12 2013 Win64
-- Check for working C compiler using: Visual Studio 12 2013 Win64 -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Found Git: C:/Git/cmd/git.exe (found version "2.6.3.windows.1")
-- Version is jemalloc-cmake.4.1-1-g7ceb9b37585bda786d87f6f9a794188ef81da226
-- jemalloc_version_major: jemalloc
-- jemalloc_version_minor: cmake
-- jemalloc_version_bugfix: 4
-- jemalloc_version_nrev: 1
-- jemalloc_version_gid: 1
-- Looking for include file alloca.h
-- Looking for include file alloca.h - not found
-- Looking for include file malloc.h
-- Looking for include file malloc.h - found
-- Looking for include file inttypes.h
-- Looking for include file inttypes.h - found
-- Looking for include file stdatomic.h
-- Looking for include file stdatomic.h - not found
-- Looking for include file sys/time.h
-- Looking for include file sys/time.h - not found
-- Looking for sys/types.h
-- Looking for sys/types.h - found
-- Looking for stdint.h
-- Looking for stdint.h - found
-- Looking for stddef.h
-- Looking for stddef.h - found
-- Check size of void*
-- Check size of void* - done
-- void* size is 8
-- Check size of int
-- Check size of int - done
-- int size is 4
-- Check size of long
-- Check size of long - done
-- long size is 4
-- Check size of long long
-- Check size of long long - done
-- long long size is 8
-- Check size of intmax_t
-- Check size of intmax_t - done
-- intmax_t size is 8
-- System pages size 4096
-- Creating public header C:/dev/oss/jemalloc-cmake/include/jemalloc/jemalloc.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc.h.header
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_defs.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_rename.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_macros.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_protos.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_typedefs.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc_mangle.h
C:\dev\oss\jemalloc-cmake\include\jemalloc\jemalloc.h.footer
1 file(s) copied.
-- Please wait while we configure class sizes

-- Finished configuring class sizes

-- Configuring done
-- Generating done
-- Build files have been written to: C:/dev/oss/jemalloc-cmake/build

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Mar 25, 2016

@dibyendumajumdar Apparently there is a build issue with a custom prefix. Will look at it when I get a chance

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Apr 6, 2016

@jasone Got some time for properly implementing with-prefix option and I am hitting a problem while building test/src for JEMALLOC_UNIT_TEST. Fixed a few issues and now things generate identically to the reference build I have from autoconf.

So I define mine_ prefix that goes into JEMALLOC_PREFIX

The main library, JET library and JEMALLOC_INTEGRATION_TEST build like a charm with JEMALLOC_MANGLE defined for the custom prefix.

However, JEMALLOC_UNIT_TEST stumbles.

c:\dev\oss\jemalloc-cmake\test\src\test.c(76): warning C4013: 'mine_nallocx' undefined; assuming extern returning int

Meaning, the compilation is lacking a proper proto at that point although it correctly wants to refer the mangled symbol. The prototypes are renamed at the rename section at jemalloc.h but in this case renaming is disabled so we get the default protos with je_ prefix.

I can not easily find what is missing in my build.

The sequence of includes appear to do the following (this is jemalloc-cmake)

test/jemalloc_test.h under JEMALLOC_UNIT_TEST defines JEMALLOC_JET and JEMALLOC_MANGLE and go on to include jemalloc/internal/jemalloc_internal.h on line 75

In that header line 12 we go into JEMALLOC_JET section and define JEMALLOC_NO_RENAME before including public jemalloc.h. JEMALLOC_NO_RENAME disables renaming je_ to mine_ so protos remain with je_ and this results in the above compile error.

What is missing?

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Apr 6, 2016

Built microbench with jemalloc-cmake. This is what I get. If anyone is willing to comment on this or share their numbers would be great.

c:\dev\oss\jemalloc-cmake\build\Release>microbench.exe
100000000 iterations, malloc=6109464us, mallocx=6828211us, ratio=1:0.895
test_malloc_vs_mallocx: pass
100000000 iterations, free=6109468us, dallocx=7562563us, ratio=1:0.808
test_free_vs_dallocx: pass
100000000 iterations, dallocx=7661670us, sdallocx=6390760us, ratio=1:1.199
test_dallocx_vs_sdallocx: pass
100000000 iterations, malloc_usable_size=6687582us, sallocx=6656350us, ratio=1:1.005
test_mus_vs_sallocx: pass
100000000 iterations, sallocx=6625113us, nallocx=6703213us, ratio=1:0.988
test_sallocx_vs_nallocx: pass
--- pass: 5/5, skip: 0/5, fail: 0/5 ---

@yuslepukhin

This comment has been minimized.

Copy link
Contributor

@yuslepukhin yuslepukhin commented Apr 7, 2016

@dibyendumajumdar Prefixed build now builds, at least the main library, however, we may need to disable building test libs since testing can be done in a normal build. Also for prefixed build you may want to define install_siffix so you do not confuse headers and libs when linking to the same project. I have not tested it but you are welcome to contribute. Along with it you will want to define private_namespace prefix as well. Shared library (DLL) build is not there yet but easy to add.

@noywuj

This comment has been minimized.

Copy link

@noywuj noywuj commented Jan 31, 2018

Your requirements are perfectly acceptable! I am working on converting a huge multi project, multi platform code base using an archaic build system to CMake. Making the switch is very difficult when you have to please 50+ engineers so I look forward to the difficulties 😄. This has also involved educating a lot of teams on CMake so I will also work with you and document enough so we can all understand how it works.

@davidtgoldblatt

This comment has been minimized.

Copy link
Member

@davidtgoldblatt davidtgoldblatt commented Jan 31, 2018

More thoughts, from a half-written reply before Jason jumped in:

Some litmus tests for a CMake build system:

  • Does it support all the current configure options?
  • Is it portable? (I think it's OK to merge without e.g. Cray support or whatever, but I'd want to think that porting there would be at least possible).
  • Does it support all current build targets? (Tests, docs, etc.)
  • Can we build all the versions of the library in a single step (my memory is that CMake is weirdly bad at producing, say, libjemalloc.so and libjemalloc.a at once).

I think this is more or less unconnected to C++, with the caveat that name mangling gets much easier after conversion (when we can put everything internal into a C++ namespace).

@mattsta

This comment has been minimized.

Copy link

@mattsta mattsta commented Mar 28, 2019

Hello again jemalloc/CMake enthusiasts!

I just spent a week updating my 2017 CMake additions to be origin/dev compliant in anticipation of the upcoming 5.2 release.

The new 2019 CMake build system is currently around 2,000 lines (while configure.ac and Makefile.in are around 3,000 lines together).

Watch it build right here in a 43 second video (context), or try it yourself:

git clone https://github.com/mattsta/jemalloc
cd jemalloc
mkdir build
cd build
cmake ..
make -j12

New Features

We are now feature-parity with the following configure.ac features:

  • automatic symbol extraction (je_ and jet_)
    • auto-generate awk formatting files
    • build symbol extraction objects with -DJEMALLOC_NO_PRIVATE_NAMESPACE (and also -O0 so they compile faster since they aren't used for linking)
    • run nm -a against object files and pipe through awk
    • output private header files
  • platform feature detection
    • tests adapted from configure.ac into CMake check_c_source_runs()
  • header generation from .h.in to .h based on config-discovered features
  • 4-way building
    • generate je_ objects with symbols
    • generate je_ objects for building
    • generate jet_ objects with symbols
    • generate jet_ objects for building
  • make test (currently only exposed when CMake release is Debug, can be adjusted to taste)
  • C++14 wrapper built and included in library
  • per-OS feature flags
  • user configuration options (the old ./configure --[opt] settings)
    • many options implemented, but not all yet
      • and the implemented options haven't all been tested either
    • options can be edited after configuration with make edit_cache
      • but may require a clean build/ directory because we aren't
        regenerating feature headers on each post-config-config update

New features of the CMake integration:

Using in other projects

The main benefit of a CMake build system is how it works as a module system for C/C++ projects.

To include jemalloc in another CMake-controlled project, just add your jemalloc dependency directory similar to:

add_subdirectory(deps/jemalloc)

But, because of the new out-of-source builds, jemalloc.h will be in build/ somewhere, so your top level project (the one including jemalloc) should add a new include path for CMake to resolve so you can still do a typical #include "jemalloc/jemalloc.h":

include_directories("$<TARGET_FILE_DIR:jemalloc-static>/../include")

Overall status

This is a huge improvement over the previous CMake addition and can be used by any CMake-built projects using jemalloc going forward.

But, even with all these changes, we still don't meet ever checkpoint David pointed out in the previous comment to completely replace the autoconf system:

Does it support all the current configure options?

Kinda. It can, many have been ported, not all have been tested, but it's easy to add and adjust.

See the current CMake user configuration port which was just a grep of AS_HELP_STRING from configure.ac with useful ones added and boring ones ignored for now.

Is it portable? (I think it's OK to merge without e.g. Cray support or whatever, but I'd want to think that porting there would be at least possible).

Yeah, that's not a problem. CMake even ships with built-in Cray compiler feature detectors if anybody wants to build that out or convert it all over from configure.ac too.

Does it support all current build targets? (Tests, docs, etc.)

Kinda. It supports regular (static and shared generation) and test targets and test running. Haven't added doc generation or install support yet.

Can we build all the versions of the library in a single step (my memory is that CMake is weirdly bad at producing, say, libjemalloc.so and libjemalloc.a at once).

Yeah, CMake doesn't have a problem building multiple things at once. It's easy as two lines of CMake.

Conclusion

CMake + jemalloc = 💖

The new add/cmake-2019 build system works everywhere I need it, but some basic features like doc generation or install targets aren't part of my world.

It should be easy to add missing features/targets and I'm happy to accept pull requests and manage improvements over at mattsta/jemalloc if anybody feels so inclined.

@wbierman

This comment has been minimized.

Copy link

@wbierman wbierman commented Mar 28, 2019

That's awesome! It looks like I still have permissions on https://github.com/jemalloc/jemalloc-cmake if you'd like to submit a merge request there.

@davidtgoldblatt

This comment has been minimized.

Copy link
Member

@davidtgoldblatt davidtgoldblatt commented Mar 28, 2019

Wow, this is amazing. I love the overall structure, the degree of completeness, and the the fact that the build system actually seems refactorable now.

TBH, I think I'd be OK merging as is after the 5.2 release cut, and figuring out the remaining feature stuff after the fact.

@mattsta

This comment has been minimized.

Copy link

@mattsta mattsta commented Mar 30, 2019

Thanks for all the positive feedback!

I'll continue rebasing my branch against origin/dev to keep it verified working for a clean merge sometime in the next couple months.

@interwq interwq modified the milestones: 5.2.0, 6.0 Mar 30, 2019
@asl

This comment has been minimized.

Copy link

@asl asl commented Mar 30, 2019

@mattsta Please note that platform defaults for "retain" option is currently missed in cmake. This is quite important for Linux builds :)

@mattsta

This comment has been minimized.

Copy link

@mattsta mattsta commented Apr 1, 2019

Please note that platform defaults for "retain" option is currently missed in cmake.

Noted! Thanks to your suggestion, retain is now fixed. I spent this weekend improving the overall CMake configuration because of all the public interest. The latest commit fixes retain and also adds retain support for 64-bit freebsd per new comments in #1339, plus adds the following fixes/improvements (tested clean on both CentOS 7.6(ish) and macOS 10.14.x):

  • custom namespace prefixes now work
  • generating non-prefixed functions for libc overrides now works
  • wrap_syms is now populated correctly
  • fixed some bugs and/or missing checks in system feature detection when compared against existing ./configure generated headers
  • enabled background_thread setting
  • added detection for linking against pthread, rt, m, dl
  • various other style/correctness improvements

If anybody notices major or minor problems with the latest CMake build commits, let me know and we can work through the fixes together.

As of the latest commit, the CMake build system generates the same header defines as the configure.ac system, and you can check any differences yourself with:

cd jemalloc
./configure
mkdir build
cd build
cmake ..
cd include/jemalloc/internal/
for f in *.h; do colordiff -u $f ../../../../include/jemalloc/internal/$f; done
cd ..
for f in *.h; do colordiff -u $f ../../../include/jemalloc/$f; done
@interwq

This comment has been minimized.

Copy link
Member

@interwq interwq commented Apr 12, 2019

A quick update, we plan to merge @mattsta 's branch in a few weeks -- delay the merging for a while mainly because we just released 5.2, and in case there needs to be a bugfix release (so far doesn't look so), we'd like to keep the commit history clean (as a major change to the build system won't really fit into a bugfix release).

@mattsta

This comment has been minimized.

Copy link

@mattsta mattsta commented Apr 18, 2019

Great to hear!

I'm still rebasing and adding features at my cmake branch every so often. The build changes can be cleanly squashed to one commit when it's time to merge.

Also I want to write up a short guide about why certain cmake decisions and style choices were made during the conversion too. Hopefully I can get those details out within a week or so.

@mattsta

This comment has been minimized.

Copy link

@mattsta mattsta commented Apr 28, 2019

Some notes about reasons behind the CMake infrastructure:

Why

CMake is useful.

CMake can act as a module system for C projects, where, if you want to add a dependency to your project and both use CMake, all you have to do is add_subdirectory(newProject) and CMake will recursively configure the subdirectory and make any CMake targets created in the subdirectory available for linking against.

This means, with jemalloc using CMake, a project can add jemalloc to its own build hierarchy then statically link against jemalloc by basically:

  • add_subdirectory(deps/jemalloc)
  • target_link_libraries(yourProject jemalloc-static)

That's it. Two lines. Then CMake will properly manage the build-order, multi-project dependency generation, and final linking of everything together.

Also, CMake manages build directory locations for everything, which allows for ideal out-of-tree builds without polluting source directories—and the out-of-tree build mechanism is enforced by CMake too, so if any part of the build attempts to modify a source directory you get a build error (this also simplifies .gitignore; just ignore build/ and you've cleared all build artifacts from source control).

It's kinda nice.

Style

Case

CMake commands are case insensitive, and polite CMake files use lowercase for commands. So, we use things like add_subdirectory(src) and not ADD_SUBDIRECTORY(src). Historically, uppercase commands used to be "standard CMake," but nobody prefers that syntax anymore.

Parens

CMake's if() syntax requires an explicit endif() command and also has parens on a final else() command.

Standard CMake if() syntax means we put the paren directly against if() (instead of a space between like if (CONDITION)), otherwise for consistency we'd also need spaces in else () and endif () and those style choices don't make sense.

Closing tag indicators

Legacy CMake supports labeling a final else() or endif() or endfunction() or endmacro() with the initial condition (e.g. if(ABC) ... else(ABC) ... endif(ABC) is equivalent to if(ABC) ... else() ... endif()), but modern cmake doesn't need the redundant statement labels, so we omit redundant conditions when they clearly aren't needed (this also helps with refactoring so you don't have to update conditions in 2 or 3 places every time you make a change).

Scope

CMake scope works in three ways:

  • global
  • local to a function
  • local to a current directory and all subdirectories

Global scope is for compile artifacts: executables, libraries, etc. When you define a final library or executable, all CMake files at any level of the CMake directory hierarchy can see the named library or executable. Also, since target scopes are global, all target names should be namespaced to the project so anybody including the project as a dependency won't generate a name conflict (example: naming an executable target stats is bad because it's too generic, so jemalloc-stats is better).

Local function scope is special because CMake functions can't return values. The way functions return results to their callers is: the caller passes in the name of a result variable, then the called function uses set(<variable> <value> PARENT_SCOPE) to escape the local function scope and modify something in the caller's scope.

Other than those two, when you set a variable in CMake, it can only be seen by the current directory and child directories, so you don't need to worry about polluting parent directory variables when creating or modifying vars.

File Organization

We're using a proper CMake layout where each directory has a CMakeLists.txt file specifying either subdirectories to add or direct build rules.

% find jemalloc -name CMakeLists.txt
jemalloc/CMakeLists.txt
jemalloc/src/CMakeLists.txt
jemalloc/test/CMakeLists.txt
jemalloc/test/integration/CMakeLists.txt
jemalloc/test/src/CMakeLists.txt
jemalloc/test/stress/CMakeLists.txt
jemalloc/test/unit/CMakeLists.txt

We store auxiliary build helper utilities in build-aux:

% find jemalloc -name \*.cmake
jemalloc/build-aux/GetSystemAddrBits.cmake
jemalloc/build-aux/UserCompileOptions.cmake
jemalloc/build-aux/DetectCompilerFeatures.cmake
jemalloc/build-aux/DetectOSFeatures.cmake
jemalloc/build-aux/Utilities.cmake
jemalloc/build-aux/GetSystemSizes.cmake

CMake embraces reusability through its include() command, which simply takes another CMake syntax file and includes it in place of the include() command.

Since include() is basically a direct file insert, the order of include() commands matter. If one include defines a variable used in a later include, they must be populated in the correct order.

All extended ships-with-CMake functionality is just include() directives of plain text CMake files too. If built-in extended features don't work quite right, you can just copy them locally, modify with fixes, and ship them internal to your own build system.

Walkthrough of a Build

User Configuration

User configuration variables have been refactored into UserCompileOptions.cmake.

User configuration can be set in two ways:

  • Passing variables to the initial cmake build:
    mkdir build
    cd build
    cmake .. -DJEMALLOC_BUILD_SHARED=OFF -DJEMALLOC_STATS_DISABLE=ON
  • or, after a cmake .. has been run, you can use the built-in cmake GUI to modify variables after the fact and CMake will re-run configuration where appropriate:
    make edit_cache
    ...
    make

Yes, this means the classic format of configure --enable-X --disable-Y doesn't work anymore and everything will get slightly more verbose at configure time (but one could always write a pass-through wrapper shell script to auto-translate configure syntax to CMake command line syntax). Though, CMake supports multiple gui-driven config manipulators too, so not all is lost.

I've attempted to keep the same configure.ac options with the same functionality, but moving forward, it would be nice to only specify ENABLE options and use proper defaults (since CMake has the concept of default values capable of being modified by the user after the fact).

Instead of having DISABLE options where we need to "negate" the user's choice to figure out if the option is enabled or not (e.g. "DISABLE" options imply we verify enable state by checking "NOT DISABLED" so we have "NOT DEBUG? OFF/[ON]" instead of "DEBUG? ON/[OFF]" (example)).

System Detection

System page size and cache line sizes are determined by running a C program at configure time from GetSystemSizes.cmake.

System virtual address bits are determined by running GetSystemAddrBits.cmake

Compiler and OS feature detectors are determined by running DetectCompilerFeatures.cmake which has most of the tiny C test programs from configure.ac imported already.

Header Generation

Some of the more esoteric parts of jemalloc configuration are handled by Utilities.cmake which was originally published Feb 29, 2016 over in jemalloc-cmake as a windows-only CMake build system.

The original Utilities.cmake was written to support multiple-OS jemalloc builds by implementing jemalloc configure-time shell scripts as CMake string manipulation functions capable of running 100% within the CMake build system without any unix tools involvement.

Since then though, the jemalloc build system has changed a lot and I've had to either discard or rewrite most of the original Utilities.cmake, but it did provide a hugely helpful starting point covering most of the hyper-specific jemalloc build requirements.

Now Utilities.cmake is primarily responsible for:

  • creating public namespace header
  • creating private namespace header
  • create rename, mangle, mangle_jet headers
  • create public symbols list
  • creating jemalloc version string from git hash
  • power of two math reductions

Another major improvement to the original CMake header generation I managed to wrangle: previously, every update to any CMake file would cause a complete jemalloc rebuild because jemalloc is using headers-generated-by-configure with the typical #undef VAR headers, and each of those header defined variables is generated from a CMake variable.

So, every time any CMake file changes, CMake wants to run all configure_file() header generation commands again because maybe the CMake variables changed and need new output. But, rebuild-all-on-cmake-update is disturbing to builds because CMake re-generates the headers, then every jemalloc source depends on the common headers generated, causing a full source rebuild, even if nothing changed.

Now, instead of using CMake's default regenerate-all-headers-on-any-cmake-change behavior, header generation is guarded with multiple IS_NEWER_THAN checks to verify the sources have changed before re-generating headers.

Unit Tests

All jemalloc tests require approximately the same build configuration:

  • build source
  • link against jemalloc
  • link against jet_
  • link against IntegrationTest

So, I created a "create test executable" function for easily creating new test executable targets instead of duplicating all those configuration behaviors in each test directory configuration.

Now each jemalloc test directory iterates over each test and calls createTest(name source)

The createTest() function populates a (correctly namespaced) target jemalloc-test-$name, so after createTest() returns, the target can still be modified (extra libraries added like needing to link libm against one test specifically).

jemalloc tests are built when CMAKE_BUILD_TYPE is Debug. Tests are not built for undefined build types or Release build types (but the when-to-build-tests is just controlled by one if check, so it's easy to change based on your own preferences).

Building jemalloc

The primary jemalloc build is controlled by src/CMakeLists.txt which provides:

  • nm/awk symbol extraction
  • dynamic creation of private_namespace.h in a safe way to avoid accidental complete rebuilds
  • creation of jet_ and unit tests when in CMAKE_BUILD_TYPE =Debug mode
  • building shared jemalloc.so (can be disabled with user option)
  • building static jemalloc.a (can be disabled with user option, unless testing is enabled)
  • automatically including C++14 helper (which, when added, will make CMake think jemalloc is a C++ project—set JEMALLOC_CXX_DISABLE to TRUE to have CMake treat jemalloc as a C-only project)

The nm/awk extraction requires some special attention because there's two ways CMake can run it:

  • all-for-one approach where any changed source file triggers all the symbols to be re-extracted
  • one-for-one approach where each symbol extraction just depends on its parent source file

The one-for-one approach sadly causes CMake to generate excessively noisy extra targets for outputting files, so that option isn't enabled currently (though the skeleton of its implementation is still in the directory).

The all-for-one approach is cleaner for CMake to handle internally, but the downside is any source change results in CMake showing status output for each extracted file again. Though, in this case, the noise is optional. If we remove the COMMENT option, CMake will still runs the commands but not provide any user status output.

Missing Features

Primary missing features of the current CMake build system versus the configure.ac setup:

Installing

CMake has built-in support for custom make install behavior, but I haven't added any files for the install target yet.

Documentation building

Just not hooked up to anything yet.

All user configuration options

The CMake user options at UserCompileOptions.cmake were extracted by grep AS_HELP_STRING configure.ac, so all options are listed, but not all options are hooked up to usable behavior yet.

Notably, the profiling and libunwind locations aren't being set or populated for downstream build integration yet.

Conclusion

Overall, this new CMake build system is about the best we can get:

  • it's a more understandable and refactorable system than endless configure-style shell scripts
  • operations can be hoisted into functions or external includes to improve readability
  • platform defines can be controlled in a hopefully more understandable way than before

I've been working on (and using) this jemalloc build system since early 2017 and have spent ~200 hours across understanding how things should work, getting them working, fixing builds when jemalloc switches underlying build expectations (symbol extraction, etc), then cleaning everything up a bit nicer for multi-project maintainability.

Having CMake-in-jemalloc is great for basically zero-configuration jemalloc integration into any CMake-built project. Now adding a high performance memory allocator to anybody's [CMake] project is as easy as two lines of build configuration and then the world becomes a slightly better place.

@leezu

This comment has been minimized.

Copy link

@leezu leezu commented Dec 18, 2019

A quick update, we plan to merge @mattsta 's branch in a few weeks -- delay the merging for a while mainly because we just released 5.2, and in case there needs to be a bugfix release (so far doesn't look so), we'd like to keep the commit history clean (as a major change to the build system won't really fit into a bugfix release).

@interwq is there any updated plan?

Debian stable now carries jemalloc 5. Projects relying on jemalloc built with --disable-initial-exec-tls need to integrate jemalloc compilation into their build setup; CMake support simplies that.

@interwq

This comment has been minimized.

Copy link
Member

@interwq interwq commented Dec 18, 2019

We are ready to merge; will need @mattsta's help to update again and submit the pull request. However I don't think we will have another versioned released with just the CMake update. It would be bundled into the next major release (late 2020 is my best guess).

On a side note, why is the --disable-initial-exec-tls needed there @leezu? (let's discuss this in a separate thread) cc: @davidtgoldblatt

leezu added a commit to leezu/mxnet that referenced this issue Dec 19, 2019
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
leezu added a commit to leezu/mxnet that referenced this issue Dec 19, 2019
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
@leezu

This comment has been minimized.

Copy link

@leezu leezu commented Dec 19, 2019

@mattsta Thanks for your work on adding CMake support!
I added a patch for the equivalent of --disable-initial-exec-tls at https://github.com/leezu/jemalloc/tree/add/cmake-2019
Also I noticed that the Debug build generates an error: multiple rules generate 3rdparty/jemalloc/src/syms/hash.c.o.sym.jet [-w dupbuild=err] (at least with cmake -GNinja)

leezu added a commit to leezu/mxnet that referenced this issue Dec 19, 2019
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
leezu added a commit to leezu/mxnet that referenced this issue Dec 19, 2019
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
leezu added a commit to leezu/mxnet that referenced this issue Dec 26, 2019
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
@madebr

This comment has been minimized.

Copy link

@madebr madebr commented Feb 4, 2020

@leezu
I just tried your cmake-2019 branch for building jemalloc on Windows using Visual Studio.

I had to make some changes to make it configure and partially compile, but not fully.

DetectOSFeatures fails to detect the Windows platform.

During build, the final step failed because nm and awk are generally not available on Windows.
The warning that is shown during this failure is also awkward.
Because ${CMAKE_NM} is empty, cmake thinks -a is the name of the application.
Also, because JEMALLOC_AWK_EXEC is undefined, JEMALLOC_AWK_EXEC-NOTFOUND is shown in the log.
The configure step should fail when these 2 variables are undefined.
But better, the default configuration options should be that it just works.

I would like to suggest some additional changes:

  • Add support for install.
  • Make use of cmake targets. So no global include_directories or add_definitions.
    This also allows the install step to install cmake scripts that export these cmake targets.
  • No use of linux specific utilities. The current sln project can build jemalloc on Windows, so I don't see why cmake cannot. cmake has a script mode which might be useful.
  • Wrap all calls to option and store the decsription, value and default in a list(s). This allows cmake to print a summary at the end of configuration. SDL2 does something similar.
  • CI for Windows, as well as Linux and Macos (using appveyor and travis-ci)?
Patch to get things going on Visual Studio 2017

These changes are just hacks to get things working. I did not use the proper cmake variables.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 49d5439c..693c667a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -16,6 +16,9 @@ include(${PROJECT_SOURCE_DIR}/build-aux/DetectCompilerFeatures.cmake) # 2
 
 # Tell CMake where to find default headers:
 include_directories(${PROJECT_SOURCE_DIR}/include)
+if(MSVC)
+    include_directories(${PROJECT_SOURCE_DIR}/include/msvc_compat)
+endif()
 
 # Also tell CMake where to find out-of-source generated headers:
 include_directories(${PROJECT_BINARY_DIR}/include)
diff --git a/build-aux/DetectCompilerFeatures.cmake b/build-aux/DetectCompilerFeatures.cmake
index d764428c..822a271b 100644
--- a/build-aux/DetectCompilerFeatures.cmake
+++ b/build-aux/DetectCompilerFeatures.cmake
@@ -151,7 +151,7 @@ int main(void) {
     mach_absolute_time();
     return 0;
 }" JEMALLOC_HAVE_MACH_ABSOLUTE_TIME)
-
+set(CMAKE_REQUIRED_INCLUDES "${PROJECT_SOURCE_DIR}/include/msvc_compat")
 check_c_source_runs("
 #include <assert.h>
 #include <strings.h>
@@ -166,13 +166,14 @@ if(HAS_BUILTIN_FFSL)
     set(JEMALLOC_INTERNAL_FFSL __builtin_ffsl)
     set(JEMALLOC_INTERNAL_FFS __builtin_ffs)
 else()
+    set(CMAKE_REQUIRED_INCLUDES "${PROJECT_SOURCE_DIR}/include/msvc_compat")
     check_c_source_runs("
     #include <assert.h>
     #include <strings.h>
     int main() {
         int rv = ffsl(0x08);
         assert(rv == 4);
-        return 0);
+        return 0;
     }" HAS_FFSL)
 
     if(HAS_FFSL)
diff --git a/build-aux/DetectOSFeatures.cmake b/build-aux/DetectOSFeatures.cmake
index 523db5ff..de0a5911 100644
--- a/build-aux/DetectOSFeatures.cmake
+++ b/build-aux/DetectOSFeatures.cmake
@@ -76,8 +76,7 @@ else()
 endif()
 
 # Only run check if we haven't run it before
-if(CMAKE_SYSTEM_PROCESSOR MATCHES "i686" OR
-   CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64")
+if(CMAKE_SYSTEM_PROCESSOR MATCHES "(i686|x86_64|AMD64)")
     check_c_source_compiles("
 int main(void) {
     __asm__ volatile(\"pause\");
@@ -118,7 +117,7 @@ endif()
 if(NOT LG_VADDR)
     if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64") # maybe "arm64" too?
         set(LG_VADDR 48)
-    elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "x86_64")
+    elseif(CMAKE_SYSTEM_PROCESSOR MATCHES "(x86_64|AMD64)")
         GetSystemAddrBits(LG_VADDR)
         # Cache result so we don't run the check every time
         set(LG_VADDR ${LG_VADDR} CACHE INTERNAL "System Address Bits")
diff --git a/build-aux/GetSystemSizes.cmake b/build-aux/GetSystemSizes.cmake
index 0baf3296..35d3a924 100644
--- a/build-aux/GetSystemSizes.cmake
+++ b/build-aux/GetSystemSizes.cmake
@@ -41,7 +41,7 @@ int main() {
         SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer = 0;
 
         GetSystemInfo(&si);
-        result = si.dwSizes;
+        result = si.dwPageSize;
     #else
         result = sysconf(_SC_PAGESIZE);
     #endif
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index a3cceb4d..bd3d56f0 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -59,7 +59,7 @@ if(NOT JEMALLOC_CXX_DISABLE)
     # C++14 support exists, so add C++ file and configure C++ options
     list(APPEND JEMALLOC_CMAKE_SOURCES
         jemalloc_cpp.cpp)
-    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -g3 -O3")
+    #set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -g3 -O3")
 endif()
 
 if(enable_valgrind)
@@ -75,7 +75,7 @@ find_program(JEMALLOC_AWK_EXEC awk)
 function(ExtractSymbolsGenerateNamespaceHeader target top-target symExt awk dir hdr)
 # Don't compile -syms with optimization so it builds faster.
 # The target is only for symbol extraction so we don't care about performance.
-target_compile_options(${target} PRIVATE "-O0")
+# target_compile_options(${target} PRIVATE "-O0")
 target_compile_definitions(${target} PRIVATE
     JEMALLOC_NO_PRIVATE_NAMESPACE
     _GNU_SOURCE)
diff --git a/src/jemalloc_cpp.cpp b/src/jemalloc_cpp.cpp
index da0441a7..01abe32d 100644
--- a/src/jemalloc_cpp.cpp
+++ b/src/jemalloc_cpp.cpp
@@ -67,7 +67,7 @@ handleOOM(std::size_t size, bool nothrow) {
 	}
 
 	if (ptr == nullptr && !nothrow)
-		std::__throw_bad_alloc();
+		std::bad_alloc();
 	return ptr;
 }
 

These changes can be summed up as:

  • include include/msvc_compat as well when using the MSVC compiler
  • when calling check_c_source_compiles and check_c_source_runs, set CMAKE_REQUIRED_* variables. I suggest wrapping these functions.
  • CMAKE_SYSTEM_PROCESSOR is AMD64 on Windows using MSVC
  • 2 bugs in C code (embedded in cmake)
  • not adding gcc/clang options to CMAKE_{LANG}_FLAGS
  • std::__throw_bad_alloc in src/jemalloc_cpp.cpp is not defined in MSVC2017.

Great work!!

@leezu

This comment has been minimized.

Copy link

@leezu leezu commented Feb 4, 2020

@madebr I only did a very small modification to @mattsta's work to verify a special use-case. @mattsta is driving this work and may be able to help.

leezu added a commit to leezu/mxnet that referenced this issue Feb 4, 2020
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
leezu added a commit to leezu/mxnet that referenced this issue Feb 4, 2020
Starting with jemalloc 5 we must build jemalloc with --disable-initial-exec-tls
to support linking libmxnet.so with libjemalloc.so

As Debian Stretch+ and Ubuntu 18.10+ ship with jemalloc 5 built without
--disable-initial-exec-tls, building MXNet with jemalloc support on any of those
platforms is currently broken.

jemalloc/jemalloc#937

To simplify integration with MXNet's CMake build, we rely on the yet to be
merged CMake version of jemalloc:
jemalloc/jemalloc#303
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.