build(linux): let the bundled jemalloc own every allocation in RocksDB - #13
kamilchodola wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Fixed-size path wrappers break valid libc behavior, and symbol artifacts would leak into generated dependency PRs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Makes bundled jemalloc own all Linux RocksDB allocations while preserving allocator safety.
Changes:
- Adds allocator and performance-context build modes.
- Adds libc allocation shims and binary verification.
- Uploads unstripped Linux libraries for symbolication.
File summaries
| File | Description |
|---|---|
AGENTS.md |
Documents the allocator configuration. |
.github/workflows/build-rocksdb.yml |
Implements allocator modes, shims, verification, and symbol artifacts. |
Review details
Suppressed comments (1)
.github/workflows/build-rocksdb.yml:190
- Replacing
realpath(path, NULL)with a fixedPATH_MAXbuffer changes its contract: glibc's dynamic form can resolve valid canonical paths longer thanPATH_MAX, while this wrapper fails withENAMETOOLONG. Route the dynamically sized result onto jemalloc without imposing this fixed limit so valid RocksDB paths do not start failing.
char *__wrap_realpath(const char *path, char *resolved)
{
if (resolved) return __real_realpath(path, resolved);
char *p = malloc(PATH_MAX);
if (!p) return NULL;
if (!__real_realpath(path, p)) { free(p); return NULL; }
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Addressed all three Copilot findings in 746d90b:
Re-running the six-platform test gate on the new shim before this is merged; will note the result here. |
|
Gate re-run on 746d90b (the Copilot fixes): 345/345 on all six platforms (run 34096795512 → test-publish on the generated branch, tests only, nothing published; gate PR #14 closed). Verify on x64: 1749 |
The library used to bundle jemalloc prefixed, serving only C++ new/delete and leaving C malloc on libc so that memory crossing the C API could never be freed by the wrong allocator. That split costs about half of the allocator's benefit: on 1000 mainnet blocks the unprefixed build spends 2.9-4.2% less CPU with unchanged latency. The hazard the split guarded against is closed instead. glibc hands RocksDB heap memory that RocksDB later frees from four calls - strdup (SaveError), getline (sysfs queue values on DB::Open), and getcwd/realpath with a NULL buffer - so those are wrapped onto shims in native/alloc_shims.c that allocate from the bundled heap. Verify asserts that the shims linked, that malloc/free are defined but unexported, and that no glibc entry point returning caller-freed memory is imported, so a RocksDB upgrade cannot reintroduce the mismatch unnoticed. The previous configuration stays available as jemalloc=private; none bundles no allocator, for diagnosis.
The shipped library is stripped and its bundled symbols are hidden, so a crash inside it cannot be symbolised from the package. Upload the pre-strip build separately so that it can.
746d90b to
9cd2617
Compare
kamilchodola
changed the title
|
Converted to draft: the CPU reduction this PR was measured on has been traced to a benchmark-harness artifact, not to the allocator. Arms whose Docker container name exceeded 63 characters ( |
Summary
Make the bundled jemalloc own every allocation inside
librocksdb.so, not only C++new/delete.Today's build (
--with-jemalloc-prefix=je_+--enable-cxx) deliberately leaves Cmalloc/freeon libc so that memory crossing the C API boundary is never freed by the wrong allocator. The hazard that guard protects against is finite and closable; the hoped-for benefit was CPU.What changes
build-rocksdb.ymlgains ajemallocinput:complete(the new default),private(today's build, kept as the rollback) andnone(no bundled allocator, for diagnosis).completebuilds jemalloc unprefixed and--wraps the libc calls whose result RocksDB frees itself onto shims innative/alloc_shims.c:strdup(SaveErrorin the C API),getline/getdelim(PosixHelper::GetQueueSysfsFileValueOfFd, on everyDB::Open), andgetcwd/realpathwhen passedNULL(ChrootFileSystemdoes that). The shims allocate from the bundled heap first and let glibc fill the buffer, so no glibc-owned pointer ever crosses an allocator boundary, which also keeps them correct when the host process interposes its own allocator throughLD_PRELOAD.malloc/freeare defined but not exported, and that no glibc entry point returning caller-freed memory is imported (strdup,getline,strndup,asprintf,backtrace_symbols,scandir,open_memstream, ...), so a RocksDB upgrade cannot reintroduce the mismatch unnoticed.symbols-linux-<arch>so a crash can be symbolised.Why the wrap is complete
The shipped library imports 258 symbols on x64 (259 on arm64). Of those, the only functions that leave malloc-owned memory in the caller's hands are
strdup,__getdelim,getcwdandrealpath; everything else that allocates (fopen,opendir,newlocale,iconv_open, ...) frees its own memory inside glibc, which can never reach the library'sfreebecause the version script keeps itlocal:.backtrace_symbolsandstrndup, which RocksDB does reference in source, are compiled out of this build and are covered by the denylist should that change. Both intermediate variants that missed a member of the set crashed deterministically in the test suite (#9 missed all four, #11 missedgetline), which is what the assertion now prevents.Tests: 345/345 on all six platforms for every build of this branch (runs 33921447156, 34096795512, 34116763361, 34129431423).
What the measurements actually showed
expb fusaka, 1000 mainnet blocks, flat layout, amd64, n=6 per arm across two mirrored dispatch orders, compared against the same Nethermind commit built with the
privatepackage, with an identical-package control image in every batch.The claim. Five batches showed the arm
jemalloc-complete(staging11.8.1-preview.112) at −2.4% to −4.6% CPU with latency and memory unchanged.The falsification. Three further
completebuilds (preview.120,.121,.123) measured within 1% of two independentprivatebuilds (.98,.92)..121was built three minutes before.112on the same commit and is byte-identical to it in code (0 differing bytes in.text; identical 22,670-function table). Swapping image tags between bit-identical images moved the "win" with the tag: the.121bits under ajemalloc-complete…tag measured −3.6%, the.112bits under a short tag −0.4%.The cause. expb names the client
expb-executor-nethermind-multi-<tag>-run<N>-nethermindand pointed Grafana Alloy's scrape target at that name. A tag of 17+ characters makes the name exceed the 63-character DNS label limit, Docker's embedded DNS cannot resolve it, Alloy never scrapes the client, and the client skips ~0.1 core of Prometheus exposition work: about 4% of run CPU, all of it in the post-benchmark idle phase, none of it in block processing. Fixed in NethermindEth/execution-payloads-benchmarks#29 (scrape through a short network alias). On the fixed harness the originaljemalloc-completeimage measures +0.3% CPU againstprivate(both orders positive).Fair verdict for
completevsprivate(all arms scraped, 30 runs ofcompleteacross five builds): CPU within ±1%, per-block latency inside the A/A floor with sign changing between batches, memory unchanged. Removing the bundled allocator entirely still costs +2.9% CPU (fair comparison), so the allocator matters; the Cmalloccalls this PR moves onto it are too few to.Risk and rollback
jemalloc=private; nothing in the managed code changes.MALLOC_CONFfrom the environment (previouslyJE_MALLOC_CONF): an operator tuning a process-wide jemalloc also tunes RocksDB's.strdup), chore(deps): update RocksDB binaries and bindings to 11.8.1 #11 (firstcomplete, crashed ongetline), chore(deps): update RocksDB binaries and bindings to 11.8.1 #12 (published to staging for measurement), chore(deps): update RocksDB binaries and bindings to 11.8.1 #14, chore(deps): update RocksDB binaries and bindings to 11.8.1 #15, chore(deps): update RocksDB binaries and bindings to 11.8.1 #16 (test gates for later builds), all closed with their diagnoses.