Improve HistoryBuf write performance by wyf-777 · Pull Request #687 · rust-embedded/heapless

wyf-777 · 2026-08-28T05:07:24Z

AI-generated text and implementation, assisted by OpenAI Codex (GPT-5).

Summary

Addresses #598 by improving HistoryBuf::write bounds-check optimization. This does not claim to fully restore heapless 0.8 performance.

Borrow the backing slice once and use the private write-position invariant for slot access.
Reject zero capacity in new_with at compile time, matching new; add a compile-fail doctest instead of the previous runtime-panic test.
Observe the complete benchmark output with black_box(&search_window).
Use bench_refs and slice iteration so input deallocation is outside timing; simplify input to 8 MiB of zero bytes.

Safety

Both constructors reject zero capacity. The private write_at starts at zero and wraps before reaching capacity. A debug assertion checks the invariant before unchecked indexing. The 14 HistoryBuf tests pass under Miri.

Corrected performance evidence

The previous 7.693 ms / 223.1 us numbers and approximately 34x claim are withdrawn: the benchmark did not observe its output and could be optimized away.

Using the same corrected benchmark and Cargo configuration on Windows x86_64, with 100 samples of 8 MiB writes:

Upstream main a50891c: median 3.757 ms.
Revised implementation: median 3.438 ms.

This single local run shows about 8.5% lower median time; timings are noisy and platform-dependent. No comparison to 0.8 or full restoration is claimed.

Validation of review changes

Full selected-feature tests (alloc, mpmc_large, portable-atomic-critical-section, serde, ufmt, bytes, zeroize, embedded-io-v0.7): 296 unit tests, 1 cpass test, 8 concurrency tests, 205 doctests passed.
14 HistoryBuf tests passed with nightly-2025-10-11 Miri and -Zmiri-ignore-leaks.
Nightly formatting and CI-equivalent i686-unknown-linux-musl Clippy passed.
Zero-capacity new_with compile-fail doctest passed.
git diff --check passed.

The current push triggers a fresh GitHub CI run. Local Windows executable tests exclude defmt; Clippy includes it.

zeenix · 2026-09-19T12:12:01Z

@wyf-777 sorry for neglecting this for week. Can you please rebase on master (instead of merging it) and then I can review it?

Reuse the backing slice and access the write slot without repeated bounds checks. The private write-position invariant keeps the unchecked access in bounds, while zero-capacity buffers continue to panic. Add a Divan benchmark for the regression reported in issue rust-embedded#598. Assisted-by: OpenAI Codex:GPT-5

Document the restored write performance in the Unreleased section.

wyf-777 · 2026-09-20T06:36:29Z

Thanks for taking the time to review! The branch is now rebased onto the latest main (a50891c), with the merge commit removed and the original fix preserved. The updated commits are pushed to this PR. Formatting, local tests (excluding defmt on Windows), and all 15 HistoryBuf Miri tests passed. The full-feature Windows test run hit unresolved defmt linker symbols, but the corresponding compilation check passed. GitHub CI currently has 20 successful checks, including the Linux test job; the full Miri job is still running. Ready for your review, thank you!

zeenix · 2026-09-20T19:01:57Z

Thanks for taking the time to review! The branch is now rebased onto the latest main (a50891c)

Np and thanks.

The full-feature Windows test run hit unresolved defmt linker symbols, but the corresponding compilation check passed. GitHub CI currently has 20 successful checks, including the Linux test job; the full Miri job is still running. R

This text was obvious generated from an LLM. Please continue to mark all generated content as such.

zeenix

Thanks for the PR. The code change itself looks fine, but the benchmark that justifies it doesn't observe its result, so the headline numbers and the changelog wording don't hold up. Details inline.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z

+
+    for byte in data {
+        search_window.write(byte);
+    }


The result of this loop is never observed, so LLVM eliminates the whole loop. black_box on the input doesn't help here since only the output matters. Running this bench locally on this branch gives a median of ~40 ns for 8 MiB, i.e. ~205 TB/s, which is not physically possible. On main the loop only survives because the bounds-check panic path keeps it alive, so the before/after compares real work against no work.

With black_box(&search_window); added after the loop, I measure ~3.79 ms on main vs ~3.05 ms on this branch, so roughly a 20% improvement rather than 34x. The bench should keep the result alive, e.g. by returning search_window.recent().copied() or via black_box.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z

+fn write_search_window(data: Vec<u8>) {
+    let mut search_window: HistoryBuf<u8, WINDOW_SIZE> = HistoryBuf::new();
+
+    for byte in data {


for byte in data consumes the Vec, so the 8 MiB deallocation (typically an munmap) lands inside the timed region and adds jitter unrelated to HistoryBuf::write. Using bench_refs with data.iter().copied() lets divan drop the input outside the measurement.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z

+use divan::counter::BytesCount;
+use heapless::HistoryBuf;
+
+const NEEDLE: &[u8] = b"needle451";


NEEDLE, bytes_before_needle, and the resize + extend don't affect the benchmark since no search is performed and the byte values are never read back. Either simplify to vec![0u8; total_bytes], or actually do the oldest_ordered().eq(NEEDLE) check from the original issue. The latter would also fix the dead-code-elimination problem above by making the result observable.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z

+        let data = self.data.borrow_mut();
+        let capacity = data.len();
+
+        assert!(capacity != 0, "cannot write to a zero-capacity HistoryBuf");


new_with is the only constructor that can produce a zero-capacity buffer, since new() already rejects N == 0 with const { assert!(N > 0) }. Such a buffer is already broken elsewhere: recent_index() computes self.capacity() - 1, which underflows for N == 0.

Adding the same const assert to new_with would fix that at the root and remove the need for this runtime check in the hot path.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z


+    #[test]
+    #[should_panic(expected = "cannot write to a zero-capacity HistoryBuf")]
+    fn write_zero_capacity_panics() {


This test locks in support for a state the rest of the type can't handle (see recent_index underflow). If new_with gets the same const assert as new, this test becomes unnecessary.

Generated by Claude Fable 5.1.

zeenix · 2026-09-20T19:12:56Z

 ## [Unreleased]

 - Fixed unsoundness in `IndexMap::retain` in the context of panicking predicate.
+- Restored `HistoryBuf::write` performance after the generic storage refactor.


With a benchmark that actually observes the result, this is a ~20% improvement over main, not a return to pre-refactor numbers. For reference, replacing get_unchecked_mut with a checked &mut data[write_at] measures the same as main, so the unsafe access is what buys that 20%. "Improved" would be more accurate than "Restored" unless a valid before/after against 0.8 is provided.

Generated by Claude Fable 5.1.

zeenix

Please address the review comments from Claude Fable.

Dear fellow heapless maintainers, I know some of you don't appreciate LLM usage for reviews but given that the PR was LLM-generated, I think it's only fair that a machine reviews it first. I have marked every single comment from the LLM clearly. I hope that's sufficient.

Observe the benchmark output and keep input destruction outside timing. Reject zero capacity in new_with and cover it with a compile-fail doctest. Correct the performance claim. Assisted-by: OpenAI Codex:GPT-5

wyf-777 · 2026-09-20T20:43:44Z

AI-generated response, assisted by OpenAI Codex (GPT-5).

Addressed the review in 7ab16a5: the benchmark now observes the output, borrows input with bench_refs, and uses a simple zero-filled input. new_with now rejects zero capacity at compile time, covered by a compile-fail doctest; the runtime check and panic test are removed.

The earlier ~34x claim was invalid and is withdrawn. With the identical corrected benchmark, this local run measured 3.757 ms on main a50891c and 3.438 ms with the fix (100 samples each). The PR title, description and changelog now say improved, not restored. Local tests, formatting, Clippy and all 14 HistoryBuf Miri tests passed. Fresh CI is running.

Thank you for catching these issues. Generated content will be explicitly labelled.

zeenix · 2026-09-21T12:23:17Z

Upstream main a50891c: median 3.757 ms.

Revised implementation: median 3.438 ms.

Then it doesn't fully address #598 at all. :(

zeenix · 2026-09-21T14:01:42Z

Then it doesn't fully address #598 at all. :(

#691 address it (almost) completely, so closing in favor of that. Please feel free to review that one.

Like `new`. A zero-capacity buffer makes `recent_index` underflow. Originally implemented by wyf-777 in PR rust-embedded#687. Assisted-by: Claude Fable 5.1 (claude-fable-5-1) Assisted-by: Claude Opus 5.5 (claude-opus-5-5)

wyf-777 added 2 commits September 20, 2026 10:38

Add HistoryBuf performance changelog entry

42d0c25

Document the restored write performance in the Unreleased section.

wyf-777 force-pushed the fix/historybuf-write-performance branch from 47e18b1 to 42d0c25 Compare September 20, 2026 06:34

zeenix reviewed Sep 20, 2026

View reviewed changes

zeenix requested changes Sep 20, 2026

View reviewed changes

history_buf: address benchmark and constructor review

7ab16a5

Observe the benchmark output and keep input destruction outside timing. Reject zero capacity in new_with and cover it with a compile-fail doctest. Correct the performance claim. Assisted-by: OpenAI Codex:GPT-5

wyf-777 changed the title ~~Restore HistoryBuf write performance~~ Improve HistoryBuf write performance Sep 20, 2026

zeenix mentioned this pull request Sep 21, 2026

HistoryBuf::write: Fix the performance regression from the storage refactor #691

Open

zeenix closed this Sep 21, 2026

Conversation

wyf-777 commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Safety

Corrected performance evidence

Validation of review changes

Uh oh!

zeenix commented Sep 19, 2026

Uh oh!

wyf-777 commented Sep 20, 2026

Uh oh!

zeenix commented Sep 20, 2026

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix Sep 20, 2026

Choose a reason for hiding this comment

Uh oh!

zeenix left a comment

Choose a reason for hiding this comment

Uh oh!

wyf-777 commented Sep 20, 2026

Uh oh!

zeenix commented Sep 21, 2026

Uh oh!

zeenix commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wyf-777 commented Aug 28, 2026 •

edited

Loading