perf(multitude): improve steady-state allocation throughput - #673
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines multitude’s steady-state allocation behavior and measurement methodology by rewinding and retaining the current chunk across Arena::reset (when safe), reducing local-reference marker overhead on hot paths, and aligning Criterion vs. Callgrind benchmarks to exercise identical allocation-free code paths. It also updates correctness tests, performance reporting, and bumps multitude to 0.8.1.
Changes:
- Rework
Arena::resetand local-reference marking to keep the common case allocation-free and minimize hot-path branching/stores. - Optimize slice copy/clone initialization paths (fixed-size copy fast path and unrolled clone with panic cleanup).
- Update benchmarks and reporting tooling to enforce allocation-free measurement boundaries and improve repeatability; update docs and versions.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/multitude/tests/arena.rs | Updates reset/cache expectations and adds new correctness regression tests for reset and slice clone/copy behavior. |
| crates/multitude/tests/alloc_ref.rs | Adjusts stats expectations/documentation to match “retained + rewound current chunk” reset semantics. |
| crates/multitude/src/internal/uninit.rs | Adds fixed-size copy fast path and rewrites clone-from-slice to an unrolled loop with panic-drop guard. |
| crates/multitude/src/internal/chunk_mutator.rs | Adds rewind support and local-reference allocation variants that mark reference handouts only on successful allocations. |
| crates/multitude/src/arena/retired_local.rs | Adds an is_empty helper for fast reset-path decisions. |
| crates/multitude/src/arena/reserve.rs | Switches local reservations to the new marker-aware allocation helpers (removing the previous explicit marker call). |
| crates/multitude/src/arena/mod.rs | Implements fast-path reset that rewinds/retains the current chunk when safe; adjusts wasted-tail stats accounting; refactors reset bookkeeping. |
| crates/multitude/src/arena/alloc_slice_ref.rs | Introduces adopt_slice_with_len helper and passes known iterator lengths through the fill-iter path. |
| crates/multitude/scripts/perf_report.rs | Adds repeated paired-comparison runs and extends teardown reporting with “reset + next allocation” diagnostics. |
| crates/multitude/docs/TODO.md | Removes completed/obsolete perf TODO items, leaving feature TODOs. |
| crates/multitude/docs/PERF.md | Updates published performance numbers and methodology text, including the new reset+allocation section. |
| crates/multitude/docs/DESIGN.md | Updates design notes to describe marker placement/cold edges and reset retention/rewind behavior. |
| crates/multitude/Cargo.toml | Bumps multitude crate version to 0.8.1. |
| crates/multitude/benches/multitude_teardown/shared.rs | Adds reset+allocate benchmark helpers using black_box. |
| crates/multitude/benches/multitude_teardown.rs | Changes teardown benchmarking harness to enforce allocation-free boundaries and adds reset+allocate variants. |
| crates/multitude/benches/multitude_teardown_cg.rs | Extends Callgrind teardown benchmarks with reset+allocate variants. |
| crates/multitude/benches/criterion_alloc.rs | Refactors Criterion benchmarks to custom timing, adds allocation-free validation, and uses an allocation-tracking global allocator. |
| crates/multitude/benches/criterion_alloc_cg/linux.rs | Updates Callgrind benchmarks to operate on mutable state references rather than move/return patterns. |
| Cargo.toml | Updates workspace dependency version for multitude to 0.8.1. |
| Cargo.lock | Updates lockfile entry for multitude to 0.8.1. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #673 +/- ##
========================================
Coverage 100.0% 100.0%
========================================
Files 503 503
Lines 57406 57585 +179
========================================
+ Hits 57406 57585 +179
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4ab01c1 to
f0879eb
Compare
Align Criterion and Callgrind comparisons around allocation-free warmed hot paths, optimize arena reset and allocation internals, add reproducible reporting and reset regressions, and bump multitude to 0.8.1. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 04fc33c2-3f61-4aed-bfbf-d0bdf32d1311
f0879eb to
5a0e757
Compare
| The marker transition and chunk-exhaustion edges are laid out as cold blocks, | ||
| leaving the marked, in-capacity path as straight-line fall-through code. | ||
|
|
||
| **Reset is a cursor rewind.** `Arena::reset` takes `&mut self`, which |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: GPT-5.6 Sol
The lifecycle diagram immediately above this section is now stale: it combines "fills up / reset" into one edge that removes CURRENT and eventually sends an Alloc-serving chunk to cache/free. The new reset path keeps a local-only current chunk installed and rewinds it in place. Could you split the refill and reset edges and show the retained/rewound-current branch so the diagram agrees with the updated prose and implementation?
| fn alloc_slice_fill_iter_raw<T, I: ExactSizeIterator<Item = T>>(&self, iter: I) -> Result<&mut [T], AllocError> { | ||
| reject_over_aligned::<T>()?; | ||
| let len = iter.len(); | ||
| fn alloc_slice_fill_iter_raw<T, I: Iterator<Item = T>>(&self, iter: I, len: usize) -> Result<&mut [T], AllocError> { |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
cargo-evaluate guideline: M-PARAMETER-CONSISTENCY
Reason:
The iterator-taking helpers place iter inconsistently relative to their closure-analog counterparts: alloc_slice_fill_iter_raw(&self, iter: I, len: usize) (line 502) puts iter first and len second, while its cold continuation alloc_slice_fill_iter_refill(&self, len: usize, refill_hint: usize, iter: I) (line 529) puts len first and iter last. This mirrors the fill_with pair (alloc_slice_fill_with_raw(len, f) / alloc_slice_fill_with_refill(len, refill_hint, f), both f-last), so the iterator variant's raw/refill split breaks the crate's own established ordering convention of placing the closure/iterator-like parameter last.
Recommendation:
Reorder alloc_slice_fill_iter_raw's parameters to (&self, len: usize, iter: I) to match the len-first, iterator-last convention used by alloc_slice_fill_iter_refill and the analogous fill_with_raw/fill_with_refill pair, and update its call site in impl_alloc_slice_fill_iter (line 490) accordingly.
Note: the len parameter is new in this PR, so this is a choice being made here rather than existing style being inherited. The unit test at line 563 also passes (iter, len) and would need the same update.
| // tail so the reported value also reflects the slack that would | ||
| // become wasted if the next alloc forced a refill right now. | ||
| let current_free = u64::from(self.current.borrow().wasted_tail_for_stats()); | ||
| let current_is_active = self.current_has_reference.get() || self.local_shared_count.get() != 0; |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
The comment directly above this line still says "Fold in the currently-active chunk's free tail so the reported value also reflects the slack that would become wasted if the next alloc forced a refill right now" — and this new line now conditionally doesn't fold it in.
The reason isn't obvious from the code: the gate exists so wasted_tail_bytes still reports zero after a reset, now that the chunk stays installed rather than being detached. Without a sentence saying that, the gate reads like a bug. Something like:
// A retained-but-rewound chunk has no live slack yet: `reset` leaves the
// chunk installed, so gate on generation activity to keep the gauge at
// zero across a reset.Worth noting that this also quietly widens current_has_reference: it now doubles as a "this chunk is active in the current generation" signal for stats(), not just "handed out an arena-lifetime reference". If that dual role is intended, the field doc is the natural place to say so.
| self.try_alloc_before_commit(size, align, || {}) | ||
| } | ||
|
|
||
| /// Local-reference form of [`Self::try_alloc`]. |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: GPT-5.6 Sol
This private comment mostly restates the helper name/link ("Local-reference form of try_alloc"). The same pattern appears on the newly added try_alloc_uninit_local, try_alloc_bytes_local, try_alloc_uninit_slice_local, and try_alloc_freezable_slice_local.
Could you replace these with the non-obvious contract instead — why the marker must transition before the cursor commit, and what that guarantees for chunk retirement? That's the thing a reader can't recover from the signature. (If the intent is specifically to disambiguate "local" as local-reference rather than thread-local, that's worth keeping — just say it once, with the invariant.)
| /// Local-reference form of [`Self::try_alloc`]. | ||
| #[inline] | ||
| #[cfg_attr(test, mutants::skip)] // see `try_alloc` | ||
| fn try_alloc_local(&self, size: usize, align: usize, marker: &Cell<bool>) -> Option<InChunk<u8>> { |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
The marker guard appears verbatim three times — here, in try_alloc_bytes_local, and in try_alloc_smart_prefixed_local:
|| {
if !marker.get() {
set_reference_marker(marker);
}
}The load-then-cold-store pattern is the actual optimization and is load-bearing for the invariant documented in DESIGN.md ("write it only for the false-to-true transition"), so it's worth having exactly one copy. You already extracted the #[cold] store; consider extracting the guard too:
/// Records the first local reservation of a generation. The load guard keeps
/// the steady state store-free; the transition itself is cold.
#[inline]
fn mark_local_reference(marker: &Cell<bool>) {
if !marker.get() {
set_reference_marker(marker);
}
}Each wrapper then becomes self.try_alloc_before_commit(size, align, || mark_local_reference(marker)).
| /// `dst` and `src` must be valid for [`FIXED_COPY_BYTES`] non-overlapping | ||
| /// bytes. | ||
| // | ||
| // Keeping this out of line is deliberate: when LLVM inlined the fixed copy |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
cargo-evaluate guideline: M-NO-META-DESIGN-DOCUMENTATION
Reason:
Two comments narrate implementation-history / 'why we picked X over Y' engineering journeys rather than documenting end-state behavior: lines 25-27 explain that keeping a function 'out of line is deliberate' because 'when LLVM inlined the fixed copy beside the dynamic fallback, it merged both paths back into libc memcpy', and lines 205-208 explain 'A conventional iterator loop made it coalesce each short trivial slice into an out-of-line memcpy; this shape instead produced inline vector copies in disassembly'. These are design-journal-style rationales about a past alternative implementation and compiler behavior observed during development, not documentation of current behavior for users.
Recommendation:
Replace the historical/comparative narrative with a concise forward-looking comment, e.g. 'Kept out-of-line and manually unrolled to ensure LLVM emits inline vector copies instead of a memcpy call; do not simplify to an iterator or #[inline] without re-checking codegen.' Avoid describing what a previous version did or why an alternative was rejected.
Note: please keep the constraint itself — that #[inline(never)] is deliberate and that this must not be simplified to an iterator loop without re-checking codegen. Only the "here's what we tried and what LLVM did" framing is at issue.
| unsafe { | ||
| let dst = slice_ptr.as_ptr().cast::<T>(); | ||
| let mut guard = InitGuard { dst, initialized: 0 }; | ||
| while len - guard.initialized >= 8 { |
There was a problem hiding this comment.
🤖 Clawpilot here! Automated review comment from an AI agent -- not a human reviewer. Please verify before acting.
AI reviewer: Claude Opus 5
cargo-evaluate guideline: M-DOCUMENTED-MAGIC
Reason:
The clone-unrolling loop uses a hardcoded unroll factor of 8 (line 214, while len - guard.initialized >= 8) with no comment explaining why 8 was chosen specifically (as opposed to 4 or 16) or what tradeoff informed it; the surrounding comment (lines 205-208) explains why manual unrolling exists at all, but not the specific magic constant 8.
Recommendation:
Add a brief comment near the unroll loop (or promote 8 to a named constant like CLONE_UNROLL_FACTOR) documenting why this specific factor was chosen (e.g., based on benchmarking or register/vector-width considerations).
Adding to that: the 8-wide step is eight byte-identical copies of the same three lines, and the literal 8 is both the loop bound and, implicitly, the body's repetition count — with nothing tying them together, so a future edit that lands in seven of eight blocks is invisible in review. Naming the constant fixes half of that; expressing the unroll as a constant-trip-count inner loop would fix both:
while len - guard.initialized >= 8 {
for _ in 0..8 {
let index = guard.initialized;
dst.add(index).write((*src.as_ptr().add(index)).clone());
guard.initialized += 1;
}
}I haven't built this, so please re-check against the disassembly you used to justify the current shape. If the nested form does regress, that's worth putting in the comment — "a constant-trip-count inner loop coalesced too" is what stops the next reader from simplifying it back.
Summary
multitudeto 0.8.1The updated report has Multitude ahead on scalar allocation, slice copy/clone, iterator fill, and growable string/vector scenarios; the two remaining scenarios are within 4% of Bumpalo.
Validation
cargo clippy -p multitude --profile dev --all-targets --all-features --lockedcargo test -p multitude --locked --all-features --test arena reset::reset_preserves_smart_owner_from_retired_mixed_chunk