Repository navigation
Conversation
derSteFfi
changed the title
derSteFfi
marked this pull request as draft
August 7, 2025 13:35
Contributor
Author
|
one ToDo I forgot in notes: tar can exclude files from extraction. The current shell-script does delete a few folders in a few repositories - we could modify the tar call to not extract them in the first place. |
derSteFfi
force-pushed
the
build_with_makefile
branch
2 times, most recently
from
September 25, 2025 14:34
1f4928c to
ffbdf87
Compare
Contributor
Author
|
the remaining CI failures are not specific to the Makefile but actual problems, so marking this ready. |
derSteFfi
marked this pull request as ready for review
October 7, 2025 13:01
jvoisin
reviewed
Oct 7, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the build_bench_env.sh script with a Makefile-based build system to improve consistency and maintainability. The change switches from git cloning to downloading versioned archives from GitHub and centralizes version management.
Key changes:
- Replace shell script with comprehensive Makefile for building allocators and benchmarks
- Introduce VERSIONS file to centralize version management across all components
- Move package installation documentation from script to README with per-distribution instructions
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| build-bench-env.sh | Add python-six dependency for ArchLinux package installation |
| bench.sh | Update library paths and remove hardcoded version variables |
| VERSIONS | New file defining versions and sources for all allocators and benchmarks |
| README.md | Replace build script documentation with Makefile usage and detailed dependency lists |
| Makefile | New comprehensive build system with targets for all allocators and benchmarks |
| Dockerfile | Update to use Makefile instead of build script and add package installations |
| .github/workflows/all.yml | Remove Fedora exclusion from CI matrix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
mjp41
reviewed
Oct 8, 2025
mjp41
left a comment
mjp41
left a comment
Collaborator
There was a problem hiding this comment.
This is looking pretty tidy. Thanks for working on this.
mjp41
approved these changes
Oct 10, 2025
mjp41
left a comment
mjp41
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, thanks for the changes.
@jvoisin, @derSteFfi I don't want to hold these changes holding off on fixing issues for individual allocators/platforms. But I really like to only have a Green CI. Thoughts?
Contributor
Author
|
Absolutely agree. I still have at least scudo and lp on my list and will re-check all comments here afterwards. With that fixed, I would also be pro-merging to make it easier to hunt individual bugs. The branch-switching sometimes takes time, and we would have a clear stand - fixing individual things is much easier than to always have to model the whole process. |
derSteFfi
force-pushed
the
build_with_makefile
branch
from
October 13, 2025 13:22
0266c64 to
2d0082e
Compare
Contributor
Author
|
@jvoisin the MR is up-to-date with master - what do you think about merging? |
Collaborator
|
~All the tests are failing :/ |
Contributor
Author
|
sorry, missed a reference to |
Collaborator
|
There are still a bunch of failures it seems |
Contributor
Author
|
these are the same we have on master, except that fedora is deactivated in the CI on master (don't remember why). alpine base container and ubuntu/tcg fail with master, too. |
jvoisin
approved these changes
Apr 7, 2026
Collaborator
|
Then it does look good to me. I'll let @daanx hit the merge button as it's his project :) |
derSteFfi
force-pushed
the
build_with_makefile
branch
from
July 9, 2026 10:11
d0028ed to
90a3c8a
Compare
Contributor
Author
|
re-synced with master. Also added rmalloc and fixed sc. Would love to not have to resolve merge conflicts like these every few months... |
derSteFfi
changed the title
Contributor
Author
|
I missed a few bumps in the merge, should be synchronized now. |
derSteFfi
force-pushed
the
build_with_makefile
branch
from
October 26, 2026 09:39
bc691e3 to
c976d1a
Compare
Contributor
Author
|
rebased onto master, added support for versioned archives. |
Replace the build-bench-env.sh usage sections with make targets (allocs, benchmarks, individual allocators, -j behaviour), move the per-distribution package lists to match the Dockerfile, and mention VERSIONS as the single place to bump allocator versions.
Download each project into archives/<name>-<label>.tar.gz, where the label is the version tag or a short sha, and unpack from there into extern/<name>. The version is now visible on disk, and each unpack stamp records "<label> <sha>": the pin the build tree was made from. Version bumps were invisible to make before: with unversioned archive names and empty stamps, an existing extern/<name> tree was never refreshed after VERSIONS moved. Freshness is now keyed by the pin: the archive target is named after the pinned version, so editing a pin gives the project a new prerequisite and make re-fetches, re-unpacks, and rebuilds exactly that project, while everything else stays put. The archive targets are declared explicitly (static pattern rules) to keep make from treating them as intermediate files it may silently skip or delete. lp and scudo unpack by sparse git clone instead of using an archive. Pins are full shas now, and git clone -b only accepts branch names, so fetch the sha explicitly and check it out detached; a marker file named after their pin, archives/<name>-<label>.clone, triggers their re-clone and nothing else. Add a clean-archives target that sweeps stale archives and markers but keeps the pinned ones, and in the README point at VERSIONS instead of build-bench-env.sh for the versions.
The bazel tcmalloc target is a static library, so bench.sh finds no shared object to preload and runs the job with an empty allocator list. Add a linkshared cc_binary for it.
hardened_malloc maps guard pages per allocation and sh6bench keeps tens of thousands of them alive, so it needs over 140000 mappings; at the default 65530 mprotect fails and malloc returns null although memory is free.
libhoard.cpp does not define them but Heap-Layers' wrapper.cpp calls them, so libhoard.so has undefined symbols and every c++ test dies at its first sized delete. Define them as weak defaults, as the mac wrapper does.
The exclusion was for a bazel target error that is gone at the pinned version. Fedora has no bazel installed and alpine does not compile tcmalloc with clang.
derSteFfi
force-pushed
the
build_with_makefile
branch
from
November 10, 2026 16:18
c0e2d53 to
6f1ed81
Compare
The bench-env image is cached per platform (type=gha, scope=buildkit_<platform>) and later jobs reuse it on other machines, but RocksDB compiles with -march=native when PORTABLE is unset (it ends up in make_config.mk). A db_bench built on a machine with AVX-512 therefore dies with SIGILL on a runner without it, before it prints a single line. That is the rocksdb:mi and rocksdb:ff failures in the current runs: the same cached binary is gone after 0.1 s with no output on some jobs, while others pass the test. PORTABLE=1 keeps the binary runnable wherever the cache delivers it, and the measured time no longer depends on which machine built the cache: locally fillrandom takes 10.0 s native and 10.2 s portable.
A run's own output goes to <test>-<alloc>-out.txt and that file does not reach a CI log, so a test that dies says nothing beyond its name: in the failing rocksdb jobs the log shows the run starting and the next test starting 0.1 s later, with no error text anywhere. Report the exit status - naming the signal when the run was killed by one - and the tail of the output file, on stderr so buildkit records it.
build-bench-env.sh goes away in this branch, so its new hoard and tcmalloc patches are kept as files under patches/ and the Makefile applies them; the vm.max_map_count step and its comment come over unchanged.
The branch changes only build-bench-env.sh, which this branch deletes, so the merge itself is empty here; the make-world equivalent is the PORTABLE=1 in rocksdb_ENV that follows.
The same change as !33, for the make world. CI caches bench-env per platform while RocksDB compiles with -march=native when PORTABLE is unset, so a db_bench built on one CPU generation dies with SIGILL on a runner of another - that is the rocksdb:mi and rocksdb:ff failures in the current runs. Locally the portable binary costs 10.2 s against 10.0 s native on fillrandom.
Guarder resolves the real pthread_create with
dlopen("libpthread.so.0", RTLD_NOLOAD). Since glibc 2.34 that library is an
empty compatibility stub that nothing loads, so the handle comes back NULL and
dlsym(NULL, name) searches the global scope from the front - which finds
guarder's own interposer. Every pthread_create then recursed into itself until
the 128-entry thread table was full and guarder aborted with "maximum
concurrent thread limit reached (128)"; an interposer mounted in front of
libguarder.so counts 128 creations and no joins for "./mstress 4 50 25". That
is the mstressN and rptestN failure in the gd jobs on ubuntu and fedora, and
with the fallback both return to exit 0. RTLD_NEXT starts the search after
libguarder.so, at the real libc; the same file already resolves malloc and free
that way. larsonN-sized still fails, for a reason unrelated to the lookup.
The branch changes only build-bench-env.sh, which this branch deletes, so the merge itself is empty here apart from the new patches/gd.patch; the make-world call to patch follows.
The same change as 801c4bf, for the make world: apply patches/gd.patch before building guarder. Only aarch64 had its own rule here, and it would have become incomplete on the other platforms, so the rule now covers both and keeps just the getentropy fix conditional.
This file contains hidden or 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
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.
pushing this draft as a starter to get your opinions - I'll be on vacation the next weeks and won't be working on it anyway. There are a few open tasks, but the general vibe should already come across. Feedback very welcome.
Decisions made:
TODO
put archives in extra storage after versionizing them (see here)docker caches these alreadyopen Questions: