fix jemalloc linking in CI #394
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
0a0dbce
to
c04b4dc
Compare
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
c04b4dc
to
c01c537
Compare
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
c01c537
to
af241f1
Compare
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
975cbb8
to
ebf8642
Compare
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
ebf8642
to
f51b10b
Compare
Benjamin-L
changed the title
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
d6741c9
to
0f28427
Compare
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
0f28427
to
5ab4a21
Compare
Benjamin-L
commented
May 24, 2024
added 9 commits
May 23, 2024 21:16
Without setting JEMALLOC_OVERRIDE, we end up linking to two different jemalloc builds. Once dynamically, as a transitive dependency through rocksdb, and a second time to the static jemalloc that tikv-jemalloc-sys builds.
Some of the features affect nix dependencies, so we need to have a full feature list available when constructing the nix derivation. This incidentally fixes the bug where we weren't enabling jemalloc on rocksdb in CI/devshells, because jemalloc is now a default feature. It does not fix the more general class of that issue, where CI is performing an `--all-features` build in a nix devshell built for default-features. I am now passing `--no-default-features` to cargo, and having it use our unified feature list rather than duplicating the unification inside cargo.
This doesn't seem to be necessary to build, and the derivation is broken in pkgsStatic.
We're planning to add a second devshell with `all-features` for CI.
Dynamically-linked jemalloc doesn't work due to link-order issues, and we want CI to be testing a static binary anyway since that's what we're publishing in releases.
CI is running `cargo build --all-features`, so we should be passing all the features to nix as well. The only thing this currently affects is the jemalloc_prof feature, but if we add any non-default features that affect nix in the future they should also be handled correctly now.
This is causing build failures on Mac: > In file included from /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/memory/memory_allocator.cc:8: > In file included from /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/memory/jemalloc_nodump_allocator.h:11: > /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/port/jemalloc_helper.h:63:36: warning: unknown attribute '_rjem_malloc' ignored [-Wunknown-attributes] > mallocx(size_t, int) JEMALLOC_ATTR(malloc) JEMALLOC_ALLOC_SIZE(1) > ^~~~~~ > /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:412:18: note: expanded from macro 'malloc' > # define malloc je_malloc > ^~~~~~~~~ > /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:75:21: note: expanded from macro 'je_malloc' > # define je_malloc _rjem_malloc > ^~~~~~~~~~~~ > /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:183:43: note: expanded from macro 'JEMALLOC_ATTR' > # define JEMALLOC_ATTR(s) __attribute__((s)) Full build log at <https://girlboss.ceo/~strawberry/pb/ygJ3>. This is likely fixable with patches to rocksdb, but not worth it since darwin is only a dev platform.
added 4 commits
May 23, 2024 21:16
This is broken on linux, but can be used by darwin users for development, since static/jemalloc/darwin is broken.
Dynamically-linked jemalloc is broken.
Benjamin-L
force-pushed
the
fix-ci-jemalloc
branch
from
55a62cf
to
9e9ff64
Compare
Previously we were relying on NIX_CFLAGS_COMPILE, but this is not being set in static devshells. A cleaner solution for complement would likely be to build the tests in their own nix derivation instead of building them in the devshell, but this change unblocks CI for now.
girlbossceo
approved these changes
May 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This should hopefully resolve the "invalid free" error in CI for #392, and may resolve the segfault in #346.
This makes the following changes:
This does not fix the issue that dynamically-linked jemalloc builds will crash with "invalid free". This is caused by the link order, which causes glibc to be loaded before jemalloc. All of our released builds are static, and now devshells/CI are as well, so this is lower priority but I would still like to fix it.