[Benchmark] mimalloc vs jemalloc#5193
[Benchmark] mimalloc vs jemalloc#5193eval-exec wants to merge 1 commit intonervosnetwork:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes allocator experiments less invasive by switching CKB’s default allocator to mimalloc (while keeping jemalloc available via a feature flag), and adds a dedicated benchmark to compare allocator RSS/peak RSS on the existing “overall” block-processing workload.
Changes:
- Switch default allocator to
mimalloc, keepjemallocbehind--features jemalloc, and makeprofilingimplyjemalloc. - Gate jemalloc-specific memory-tracker hooks behind a dedicated
jemallocfeature. - Add
allocator_memorybenchmark and document how to run it.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| util/memory-tracker/src/process.rs | Gates jemalloc ctl usage behind feature = "jemalloc" and adjusts tracking loop logic. |
| util/memory-tracker/src/lib.rs | Updates docs/feature semantics for jemalloc profiling support. |
| util/memory-tracker/Cargo.toml | Makes jemalloc deps optional; adds jemalloc feature; profiling implies jemalloc. |
| src/main.rs | Switches global allocator selection to feature-based jemalloc/mimalloc. |
| ckb-bin/Cargo.toml | Adds jemalloc feature forwarding to memory-tracker. |
| benches/src/lib.rs | Documents allocator-memory comparison commands. |
| benches/benches/benchmarks/overall.rs | Extracts reusable run_overall_rounds helper used by new benchmark. |
| benches/benches/allocator_memory.rs | Adds new benchmark binary reading RSS/peak RSS and running the overall workload. |
| benches/Cargo.toml | Adds optional allocator deps + bench target; defaults benches to mimalloc. |
| Cargo.toml | Adds mimalloc dep/feature, makes tikv-jemallocator optional, updates feature wiring and defaults. |
| Cargo.lock | Adds mimalloc packages and updates transitive dependency resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ckb_metrics::handle().is_some() { | ||
| if let Some(tracker) = tracker_opt.clone() { | ||
| tracker.gather_memory_stats(); | ||
| } |
There was a problem hiding this comment.
ckb_metrics::handle() is called multiple times per loop iteration (for process metrics, jemalloc metrics, and the new is_some() check). Consider caching the handle once at the start of the loop iteration (e.g., let metrics = ckb_metrics::handle();) and reusing it to avoid repeated lookups and ensure consistent behavior within an iteration.
| #[cfg(feature = "jemalloc")] | ||
| #[global_allocator] | ||
| static ALLOC: tikv_jemallocator::Jemalloc = tikv_jemallocator::Jemalloc; | ||
|
|
||
| #[cfg(all(not(feature = "jemalloc"), feature = "mimalloc"))] | ||
| #[global_allocator] | ||
| static ALLOC: mimalloc::MiMalloc = mimalloc::MiMalloc; |
There was a problem hiding this comment.
tikv-jemallocator is only declared for cfg(all(not(target_env = "msvc"), not(target_os = "macos"))) in Cargo.toml, but the #[cfg(feature = "jemalloc")] global allocator here is not target-gated. Enabling the jemalloc feature on Windows/MSVC or macOS will compile this block and fail due to the missing dependency. Consider matching the cfg to the dependency target (or emitting a compile_error! on unsupported targets when feature="jemalloc" is enabled).
| fn read_memory_snapshot() -> MemorySnapshot { | ||
| let status = fs::read_to_string("/proc/self/status").expect("read /proc/self/status"); | ||
| let mut rss_bytes = None; |
There was a problem hiding this comment.
This benchmark hard-depends on Linux /proc (/proc/self/status) and will panic at runtime on non-Linux targets. Consider gating the binary with #[cfg(target_os = "linux")] (and providing a stub main that prints a clear message on other OSes) or using a cross-platform crate (e.g., sysinfo) to read RSS/peak RSS.
| //! cargo bench --bench allocator_memory --features ci | ||
| //! cargo bench --bench allocator_memory --no-default-features --features "ci jemalloc" |
There was a problem hiding this comment.
The new benchmark commands are ambiguous about where they should be run from. Earlier in this doc you use cd benches && cargo bench ..., and the PR description uses cargo bench -p ckb-benches ...; running cargo bench --bench allocator_memory from the workspace root likely won’t find this bench target. Consider updating these lines to consistently include -p ckb-benches (or cd benches && ...).
| //! cargo bench --bench allocator_memory --features ci | |
| //! cargo bench --bench allocator_memory --no-default-features --features "ci jemalloc" | |
| //! cargo bench -p ckb-benches --bench allocator_memory --features ci | |
| //! cargo bench -p ckb-benches --bench allocator_memory --no-default-features --features "ci jemalloc" |
Make mimalloc the default allocator, keep jemalloc behind a feature flag, and add an allocator memory bench for side-by-side RSS comparison. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
002543a to
951d9f8
Compare
What problem does this PR solve?
https://github.com/microsoft/mimalloc
Problem Summary:
What is changed and how it works?
What's Changed:
mimallocthe default allocator forckbjemallocavailable behind--features jemallocprofilingimplyjemalloc, so jemalloc-only profiling still builds correctlyjemallocfeaturecargo bench -p ckb-benches --bench allocator_memoryto compare allocator RSS / peak RSS on the existing overall block-processing workloadRelated changes
Check List
Tests
Observed allocator bench output from the last two commands:
after_workload rss_bytes=173170688,after_drop rss_bytes=174518272after_workload rss_bytes=125931520,after_drop rss_bytes=115007488Side effects