Switch default allocator from jemalloc to mimalloc#2057
Conversation
This PR switches the default memory allocator from jemalloc to mimalloc, resulting in 2.5x-3x performance improvement based on benchmarks. Changes: - Changed 'base' feature from 'use-jemalloc' to 'use-mimalloc' - Added 'base' to default features so mimalloc is enabled by default - Added mimalloc as the primary global allocator with platform detection - Kept jemalloc support via 'use-jemalloc' feature flag (deprecated) - Updated CHANGELOG.md, README.md, and benchmarks/README.md Performance improvement verified via benchmarks: - Recursive file listing: 2.5x faster - Pattern matching: 2.8x faster - Hidden files search: 2.6x faster - Case-insensitive search: 3.0x faster - Extension-based search: 2.7x faster All 267 tests pass with the new configuration.
tmccombs
left a comment
There was a problem hiding this comment.
This is ok for a draft to run benchmarks, but would need some changes before merging.
… remove benchmarks - Sync allocator cfg gates between Cargo.toml and src/main.rs - Merge jemalloc + mimalloc deps into single target-gated section - Remove spurious ios exclusion from mimalloc gate (not in jemalloc gate) - Use uniform not(windows) in both gates - Add author credit to CHANGELOG entry per CONTRIBUTING.md format - Restore Bugzilla + PR#481 citations in README EPEL paragraph - Remove benchmarks/ dir (hardcoded local paths, no system-allocator baseline, belongs in fd-benchmarks repo) Build: clean. Tests: 111/111 pass.
|
Addressed all review feedback in commit 9a4cefb. Per-thread resolution: Thread 1 — mimalloc availability (Cargo.toml):
Thread 2 — cfg gate inconsistency (src/main.rs):
Thread 3 — benchmarks/ directory:
Additional fixes in this commit:
Build: clean. All 111 tests pass. |
polarathene
left a comment
There was a problem hiding this comment.
This review was started before your recent changes, but still seems to have relevant feedback that you should go over.
Regarding your recent changes update:
-
Thread 1:
Build won't attempt to compile mimalloc on unsupported platforms.
Unifying the platforms means you're limiting what mimalloc actually has support on such as musl targets.
-
Thread 2:
structurally identical aside from the feature flags.
Please revert the change to the
defaultfeature set inCargo.toml. The system allocator was the default and for those other platforms that jemalloc did not support, they would compile with the system allocator. Your change here will make the builds fail by default for those platforms. -
Thread 3:
These belong in a separate PR targeting https://github.com/sharkdp/fd-benchmarks where they can be properly generalized.
Be sure to go over the prior feedback I've provided in this review. You had much more issues with that clearly untested script than the hard-coded home path.
At the
fd-benchmarksrepo you can just leverage theregression.shscript, it could be updated to take args for the binaries instead of it's hard-coded vars. You can use bash syntax to dynamically append args to a list if needing to support more than an A/B comparison (this would add extra complexity to the script to construct the command permutations/variants however).
Personally, I'm not a big fan of providing reviews like this where the author is clearly leaning on an LLM so heavily.
FWIW: I had different results where mimalloc could appear slower. I didn't see any consistent significant boost from the examples I provided.
| REBUILD=false | ||
| NUM_RUNS=20 | ||
| NUM_WARMUP=3 | ||
| SEARCH_PATH="/home/nanw/References/fd" |
There was a problem hiding this comment.
Why would you contribute this value?
| NUM_RUNS=20 | ||
| NUM_WARMUP=3 | ||
| SEARCH_PATH="/home/nanw/References/fd" | ||
| BIN_DIR="target/release-jemalloc" |
There was a problem hiding this comment.
Why is this not JEMALLOC_BIN_DIR?
Furthermore, you only ever use these two vars to add a /fd suffix, so they seem rather redundant?
| # Build jemalloc version | ||
| JEMALLOC_BIN="$BIN_DIR/fd" | ||
| echo ">>> Building fd with jemalloc..." | ||
| if [ ! -f "$JEMALLOC_BIN" ] || [ "$REBUILD" = true ]; then | ||
| cargo build --profile=release-jemalloc --features=use-jemalloc | ||
| else | ||
| echo " Using existing binary: $JEMALLOC_BIN" | ||
| fi | ||
|
|
||
| # Build mimalloc version | ||
| MIMALLOC_BIN="$MIMALLOC_BIN_DIR/fd" | ||
| echo ">>> Building fd with mimalloc..." | ||
| if [ ! -f "$MIMALLOC_BIN" ] || [ "$REBUILD" = true ]; then | ||
| cargo build --profile=release-mimalloc --features=use-mimalloc | ||
| else | ||
| echo " Using existing binary: $MIMALLOC_BIN" | ||
| fi |
There was a problem hiding this comment.
--profile is only used here to adjust the location in target/? You might as well just leverage the ENV CARGO_TARGET_DIR instead?
CARGO_TARGET_DIR=target/release-mimalloc cargo build --release --no-default-features --features use-mimallocI added --no-default-features because you've had the Cargo.toml modified to use mimalloc in the default feature set, whilst not preventing both features from being set.
| # Verify allocator linkage | ||
| echo "" | ||
| echo ">>> Allocator verification:" | ||
| JEMALLOC_CHECK=$(ldd "$JEMALLOC_BIN" 2>/dev/null | grep -i jemalloc || echo "not found") | ||
| MIMALLOC_CHECK=$(ldd "$MIMALLOC_BIN" 2>/dev/null | grep -i mimalloc || echo "not found") | ||
| echo " jemalloc binary: $JEMALLOC_CHECK" | ||
| echo " mimalloc binary: $MIMALLOC_CHECK" |
There was a problem hiding this comment.
Did you actually run this benchmark script and pay attention to the output?
Those ldd checks won't verify correctly unless the system has .so of the files in it's linker paths at link time (for example in Fedora that'd be provided via mimalloc-devel), but even then the mimalloc crate does not support dynamic linking mimalloc, it's only statically linking.
Jemalloc does have support to dynamic link but only via the JEMALLOC_OVERRIDE ENV, but that is something you cannot control on your side (the user must provide it explicitly as the location can differ). There is also no compatibility guarantee when providing such an override and as per the linked docs you'd need to opt-in to the unprefixed_malloc_on_supported_platforms feature to use unprefixed symbols (not supported on Apple systems).
EDIT: Actually it seems that you can avoid adding that feature explicitly and just use:
JEMALLOC_OVERRIDE=/usr/lib64/libjemalloc.so \
CARGO_TARGET_DIR=target/jemalloc-dyn \
cargo build --release --no-default-features \
--features use-jemalloc,tikv-jemallocator/unprefixed_malloc_on_supported_platformsThere was a problem hiding this comment.
You could verify this way, as both allocators support these ENV at runtime (should be more reliable than ldd or checking for symbols which in this case are stripped):
# `_RJEM_` prefix is required for the static compiled default.
# `MALLOC_CONF=stats_print:true` when instead using the `unprefixed_malloc_on_supported_platforms` feature.
$ _RJEM_MALLOC_CONF=stats_print:true ./target/jemalloc/release/fd --version 2>&1 | grep jemalloc
___ Begin jemalloc statistics ___
--- End jemalloc statistics ---
$ MIMALLOC_VERBOSE=1 ./target/mimalloc/release/fd --version 2>&1 | grep mimalloc
mimalloc: process init: 0x7F793E407C40
mimalloc: reserved 1048576 KiB memory
mimalloc: process done: 0x7F793E407C40| hyperfine \ | ||
| --runs "$NUM_RUNS" \ | ||
| --warmup "$NUM_WARMUP" \ | ||
| --name "fd-jemalloc" \ |
There was a problem hiding this comment.
--name is not a valid argument to hyperfine... I get the impression you've vibe coded this file and not bothered to test it.
| feature = "use-mimalloc", | ||
| not(feature = "use-jemalloc") |
There was a problem hiding this comment.
Don't do this, what happens if someone adds both --features use-mimalloc,use-jemalloc? This can happen when a change like you've added to Cargo.toml makes one a default feature.
Instead prior to this code you should add a compile error:
#[cfg(all(feature = "use-jemalloc", feature = "use-mimalloc"))]
compile_error!("Features 'use-jemalloc' and 'use-mimalloc' are mutually exclusive and cannot be enabled together.");This makes it more apparent that a mistake was done.
| # Jemalloc release profile for benchmarking | ||
| [profile.release-jemalloc] | ||
| inherits = "release" | ||
| env = ["CARGO_PROFILE_RELEASE_JEMALLOC=1"] | ||
|
|
||
| # Mimalloc release profile for benchmarking | ||
| [profile.release-mimalloc] | ||
| inherits = "release" | ||
| env = ["CARGO_PROFILE_RELEASE_MIMALLOC=1"] |
There was a problem hiding this comment.
Redundant, these don't belong here. What was the purpose for the ENV?
| base = ["use-jemalloc"] | ||
| default = ["completions"] | ||
| # Default allocator is mimalloc (cross-platform, better performance per benchmarks) | ||
| base = ["use-mimalloc"] | ||
| default = ["base", "completions"] |
There was a problem hiding this comment.
Why are you placing base into default? The default behaviour is to not use jemalloc, it would instead use the system memory allocator (eg: via libc implementations glibc/musl).
MiMalloc is not necessarily providing any notable improvement on a glibc system. It does have an advantage over musl, notably across multi-threaded workloads.
| ## Changes | ||
| - Switch default allocator from jemalloc to mimalloc for improved performance (2.5x-3x faster per benchmarks), see #2056. Users can still build with jemalloc using `--features=use-jemalloc`. |
There was a problem hiding this comment.
Default allocator never was jemalloc.
Summary
This PR switches the default memory allocator from jemalloc to mimalloc, resulting in 2.5x-3x performance improvement based on benchmarks. The allocator remains configurable via Cargo features, allowing users to still build with jemalloc if needed.
Motivation
Issue #2056 reported that mimalloc outperforms jemalloc by 2.5x-3x in benchmarks. mimalloc also has better cross-platform support and fewer known issues (e.g., jemalloc has a known bug on macOS Catalina).
Changes
Cargo.toml
basefeature fromuse-jemalloctouse-mimallocbasetodefaultfeatures so mimalloc is enabled by defaultuse-jemallocfeature flagsrc/main.rs
Documentation
Benchmark Results
Testing
use-jemallocfeatureuse-jemallocfeature