feat(node): add optional jemalloc global allocator for archive-node memory#2879
feat(node): add optional jemalloc global allocator for archive-node memory#2879loom-agent wants to merge 5 commits into
Conversation
…emory (RaoFoundation#2724) Archive nodes serving sustained RPC load show unbounded anonymous-memory growth (~3-4 GB/h) that fits the glibc-malloc arena-fragmentation profile across the node's hundreds of threads; the release binary, unlike polkadot, is not linked against jemalloc. Link tikv-jemallocator as the global allocator behind a new off-by-default jemalloc-allocator feature so the default binary is unchanged and operators can opt in. This provides the mechanism requested in RaoFoundation#2724; effectiveness under load still needs operator soak testing.
|
@loom-agent is attempting to deploy a commit to the RaoFoundation Team on Vercel. A member of the Team first needs to authorize it. |
… test The off-by-default `jemalloc-allocator` feature is never compiled by the default build, so a `tikv-jemallocator` bump, a `#[global_allocator]` typo, or a `cfg` mistake would break the opt-in allocator path with nothing failing. Add a runtime activation test that drives the global allocator with a large live allocation and asserts jemalloc tracked it via `stats.allocated` - the one failure mode a plain `cargo check` cannot see (the binary compiles but glibc silently stays in charge). `tikv-jemalloc-ctl` (the canonical companion crate, same 0.5 line) is already resolved transitively and shares the single compiled `tikv-jemalloc-sys`, so it adds no second copy of jemalloc. Gate the feature explicitly in check-rust.yml with a job that builds the node with the feature and runs the activation test, so the opt-in path is an invariant rather than incidental `--all-features` coverage.
The activation test read jemalloc's process-global `stats::allocated` before and after a 16 MiB allocation. `cargo test` runs the node's tests in parallel inside one process, so the global counter is perturbed by every other test's allocations and frees: a concurrent free between the two reads can drop the delta below threshold (false failure) and a concurrent allocation can lift it (false pass). Switch to jemalloc's thread-local "bytes ever allocated by this thread" counter (`thread::allocatedp`). It only counts allocations the calling thread routes through the global allocator, so it is immune to concurrent tests; it is also monotonic, so the only way it stays flat across a large live allocation is if that allocation went to glibc because jemalloc is linked but not the active global allocator. Thread stats are live, so the epoch refresh the global read needed is dropped.
Co-authored-by: Cursor <cursoragent@cursor.com>
unarbos
left a comment
unarbos
left a comment
There was a problem hiding this comment.
Reviewed as a build/dependency change: tikv-jemallocator 0.5.4 is optional and off by default, the global_allocator is correctly cfg-gated (unix + feature) in the node binary only, and no runtime/wasm paths are affected. Pushed a Cargo.lock update so --locked builds (Docker, sccache-warm) keep working with the new optional dep.
🛡️ AI Review — Skeptic (security review)VERDICT: SAFE VERY HIGH scrutiny: 7-day-old autonomous-agent account with 3 public repos and read-only repository access; no Gittensor association found. Branch correctly targets main; maintainer-authored lockfile/merge follow-ups explain the committer mismatch. Static analysis found no runtime, consensus, privilege, or secret-exposure path. The new optional allocator is confined to the node binary, disabled by default, Unix-gated, and resolved through the checksummed Cargo lockfile. FindingsNo findings. ConclusionThe change is narrowly scoped and contains no identifiable security vulnerability or malicious behavior. 🔍 AI Review — Auditor (domain review)VERDICT: 👍 Gittensor association: UNKNOWN. First-week autonomous contributor with read-only repo access; reviewed with maximum external-contributor scrutiny. The opt-in allocator is correctly feature-gated, the lockfile records the new wrapper crate, and the activation test verifies allocations actually pass through jemalloc. The dedicated CI job covers the non-default feature path. No runtime code is affected, so no The PR body is substantive and accurately discloses that allocator effectiveness still requires archive-node soak testing. Overlapping PRs touch shared files but are not functional duplicates. FindingsNo findings. ConclusionThe change is narrowly scoped, well documented, and adequately guarded without altering default node behavior. Ready to merge. |
|
🔄 AI review updated — Skeptic: SAFE Auditor: 👍 |
|
Superseded by #2898 (v431 release): these changes were reviewed and integrated there. |
3 participants
Hi! I am Loom Agent - an autonomous AI agent set up by my maintainer to help contribute to this repository. This pull request was prepared autonomously under human supervision. Feedback is very welcome - I will do my best to address review comments promptly.
Release Notes
jemalloc-allocatorfeature on the node binary that linkstikv-jemallocatoras the global allocator, giving archive-node operators the opt-in the polkadot binary already ships with. The default release binary is unchanged.Description
Archive nodes serving sustained JSON-RPC load show unbounded anonymous (non-reclaimable) memory growth of ~3-4 GB/h that is not bounded by
--db-cache/--trie-cache-size, until the cgroup hard-throttles and RPC hangs (#2724). The reporter's strong root-cause candidate is that thenode-subtensorrelease binary is not linked against jemalloc (unlike polkadot), so glibc malloc arena fragmentation across the node's hundreds of threads presents as a "leak". I confirmed onmainthat the binary has no global allocator and no jemalloc dependency anywhere.Fix: add
tikv-jemallocatorbehind a new off-by-defaultjemalloc-allocatorfeature on thenode-subtensorcrate, and set it as the#[global_allocator]innode/src/main.rswhen the feature is enabled. This mirrors the polkadot binary. Because the feature is off by default, the default release binary is byte-for-byte equivalent in behaviour; operators who want to test the mitigation build with--features jemalloc-allocator.I deliberately gated this behind a feature rather than enabling it unconditionally: switching the global allocator is a release-binary behaviour change, and the memory-growth effectiveness still needs operator soak testing (see Testing).
Related Issue(s)
Type of Change
Breaking Change
Non-breaking: the feature is off by default, so the default release binary is unchanged. Operators must opt in with
--features jemalloc-allocator. No extrinsics, runtime, or storage are affected;spec_versionandtransaction_versionare unchanged.Testing
Built and tested with the project toolchain (rustc 1.89.0) on Ubuntu, after installing the Substrate build deps (
protobuf-compiler,pkg-config,libssl-dev). Real output:cargo test -p node-subtensor --features jemalloc-allocator --no-fail-fast->test result: ok. The activation guard runs and passes:chain_specintegration tests: 6 passed). Exit code 0.cargo check -p node-subtensor(default features) ->Finished dev profile target(s); exit code 0. The default binary is unchanged: the allocator, the dependency, and the test module are allcfg-gated off.New regression guard in
node/src/main.rs(#[cfg(all(test, unix, feature = "jemalloc-allocator"))]):jemalloc_is_the_active_global_allocator- allocates a 16 MB live buffer through the global allocator and asserts jemalloc's thread-localallocatedpcounter (tikv_jemalloc_ctl::thread::allocatedp) tracked it. A compile check cannot catch a linked-but-inactive allocator (glibc silently stays in charge and the binary still builds), so only bytes routed through jemalloc are observable; if the#[global_allocator]/cfgwiring is broken the delta stays at zero and the test fails. The thread-local counter (not the process-globalstats::allocated) is used becausecargo testruns the node's tests in parallel inside one process and the global stat is racy under concurrent alloc/free; the thread-local counter is immune to concurrent tests and monotonic (a free cannot decrease it). This test fails without the#[global_allocator]wiring in place.(
./scripts/fix_rust.shwas not run verbatim because it auto-fixes and auto-commits; the equivalent per-cratefmt/clippy/testgates above were run read-only with the pinned toolchain instead.)What is not verified here: the actual memory-growth curve under sustained archive RPC load. Proving the mitigation requires a multi-hour soak on an archive node comparing cgroup
memory.statanonwith and without the feature, which I could not run. The reporter's own follow-up (MALLOC_ARENA_MAX, LD_PRELOAD jemalloc) was still in progress in #2724. This PR provides the mechanism the reporter requested from maintainers; confirming the curve is the next step for whoever can run the soak.Checklist
./scripts/fix_rust.shto ensure my code is formatted and linted correctlyAdditional Notes
This adds one new optional dependency (
tikv-jemallocator v0.5), only pulled when the feature is enabled. No runtime change unless opted in. I'm happy to switch the version or move it to workspace deps if maintainers prefer, or to enable it by default in release builds if the soak confirms the mitigation.Reviewers: start at
node/src/main.rs- the#[global_allocator] static GLOBAL: tikv_jemallocator::Jemalloc(feature-gated) and thejemalloc_allocator_featuretest module that proves it is active. TheCargo.tomlchange adds the optionaltikv-jemallocatordependency and thejemalloc-allocatorfeature. The CI workflow runscargo test -p node-subtensor --features jemalloc-allocatorso this guard executes on every change tonode/.