You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
perf(performables): avoid allocation in ready blocking operations - #810
Blocking channel operations and async-capable barrier waits allocate a thread waker even when they can finish immediately. This adds avoidable allocation and latency to ready operations, unlike the existing uncontended mutex and reader-writer-lock fast paths.
Approach
Attempt queue send/receive, oneshot receive, watch change, and barrier arrival directly before entering the shared blocking adapter.
Preserve existing waiting machinery for pending operations, once-per-operation contention recording, a single barrier arrival, and timeout deadlines captured before readiness checks.
Add native zero-allocation regressions, deterministic pending-path success/closure tests, a full-queue blocking-send telemetry assertion, and ready-operation benchmarks. Allocation checks live in ready_blocking_allocations.rs.
Document generation/deadline/contention handoffs, fixture setup, scheduling safeguards, and the requirement to use asynchronous waits on executor workers needed by other participants.
No public API changes relative to current main. Base integration retains main's crate-root Error, sync::{Async, Sync}, and BarrierWaitOutcome APIs from #820. Queue payload storage and enabled telemetry may allocate independently. Final async-capable barrier arrivals avoid allocating their own waiter storage; this does not promise allocation-free release of other waiters. Pending operations retain their existing blocking and cancellation behavior.
Benchmark comparison
Compared against unmodified main at 131f7d6a, with identical benchmark source, Rust 1.95, all features, fat LTO, and one logical CPU on Windows x64 / AMD Ryzen 7 PRO 7840U. Ran baseline/optimized/optimized/baseline, with 100 samples, 1-second warm-up and 2-second measurement per case. Values average the two independent-run medians. Before/after 95% confidence intervals remained separated across both repeats for every case.
Measurements were taken at original commit b3d78d3 (replayed as 6f4830e6), before later naming, documentation, test-only follow-ups, and base integrations. Reported benchmark identities and measured operations remain unchanged; synchronization type paths were adapted to #820. These timings were not rerun after the follow-ups, rebase, or merges.
Ready blocking operation
Before
After
Latency reduction
Bounded queue send
96.5 ns
29.6 ns
69.3%
Unbounded queue send
107.9 ns
38.5 ns
64.3%
Bounded queue receive
113.9 ns
36.1 ns
68.3%
Unbounded queue receive
118.4 ns
38.8 ns
67.3%
Bounded queue timed receive
156.4 ns
78.9 ns
49.5%
Unbounded queue timed receive
150.2 ns
79.1 ns
47.3%
Oneshot receive
150.5 ns
79.5 ns
47.2%
Oneshot timed receive
195.0 ns
106.1 ns
45.6%
Watch change
104.9 ns
25.5 ns
75.7%
Async-mode barrier wait
110.8 ns
31.5 ns
71.5%
These are already-ready microbenchmarks, not contended throughput or end-to-end application speedups. Independent allocation tests reproduce one thread-waker allocation per ready call before the change and zero afterward, with queue payload storage prepared outside measurement.
Base integration and CI corrections
Current head 676aeecc merges main at f660d7c6. The sole textual conflict was in Arty's worker-wait regression, where #843 independently introduced a fuller deterministic-cleanup fix. The merge retains upstream's public cancellation test and private consuming-stop regressions, including all shutdown-completion joins, and removes this branch's superseded duplicate. Arty is now identical to main; the PR is back to a 12-file diff. The optimization, allocator and Seismograph files are unchanged from the previously passing head 627e1f59. The lockfile difference remains only the performables allocation-test dependency.
Earlier integration preserved #820's diagnostics APIs/documentation and cargo-anvil 0.14.0 from #831. The generated crate README was regenerated, not edited by hand. The branch also includes the Quinn security fix from #816 and subsequent upstream lockfile updates; no advisory ignores were added.
Existing test defects were corrected without changing those crates' production behavior:
16f6f6f2 (formerly 5fb3b7dd): the allocator concurrent-publication test drains at least once before checking whether producers finished, so scheduling cannot skip its consumer loop. All original uniqueness, completeness, and tail-retention assertions remain.
ec8c2569 (formerly 84acafd8): the Seismograph clear-buffer test distinguishes active allocation reuse from valid retired-thread cleanup. An explicit retired-thread fixture reproduced the old failure deterministically: total allocation fell from 37264 to 18832 bytes, the same 18432-byte difference seen in CI. The corrected test checks active ring allocation identity/capacity across enabled/disabled clear, exact policies and event counts, and actual retired-ring release. Fallible statistics prevent observation errors from appearing as empty results. 62e0073b (formerly 75fae2ab) gives the fixtures safely owned process-lifetime references, removing both new pointer dereferences reported by CodeQL without suppressing or dismissing alerts.
The Windows Miri shutdown failure led to 627e1f59: nested runtime owners consumed on an async worker correctly request shutdown without waiting, so the regression needed retained completion handles and off-worker joins. That implementation is now superseded by the equivalent, broader upstream #843 tests, not dropped without replacement. No production runtime behavior or public API was changed by that follow-up.
d7f12241 (formerly a7b9fff0) addresses documentation/readability feedback with concise contracts, generated README updates, and worker-local handle ownership. All 14 threads from that review and both later CodeQL threads were replied to and resolved. Watchdogs and mutation cases remain enabled; no requested exclusions or watchdog disabling were introduced.
Validation
Latest integration (676aeecc)
Full workspace just anvil-clippy passes, as does explicit Rust 1.95 all-feature/all-target Clippy for arty, arty_executor, performables, rallocator and seismograph. The earlier local Spectre-library prerequisite blocker is no longer present in this validation.
Combined native suites for those five packages pass: 908 all-feature tests and 706 no-default tests, with two existing skips in each configuration. Debug and release all-feature builds pass.
Both retained cancellation-destructor regressions (private consuming-stop coverage and public integration coverage) pass the existing Anvil Miri runner across 16 scheduling seeds. The upstream self-stop regression also passes 16 seeds. Other filtered executables have no matching tests; this is not a fresh full Arty Miri suite.
Anvil formatting, generated README regeneration and spellcheck pass, with no generated-file drift. No runtime behavior, Miri leak diagnostics, assertions, resource limits or thresholds were weakened to resolve the conflict.
Fresh provider CI must validate 676aeecc; passing checks on 627e1f59 do not establish current-head success.
Earlier measured evidence (not rerun on the latest merge)
Exact Windows source coverage was measured after the prior semantic merge at a1d069ce, with zero raw uncovered LCOV source lines in both feature configurations:
Package
Exact Windows source coverage
performables
3790/3790 (100%)
rallocator
3815/3815 (100%)
seismograph
6945/6945 (100%)
Arty was measured at 1563/1563 after 627e1f59; subsequent upstream lifecycle refactoring changes its source inventory, so that historical total is not a current Arty coverage claim. Linux coverage passed at 100% before the base integrations and must be reconfirmed by fresh CI. No fresh complete local Anvil PR-tier success is claimed.
Both channel and barrier unit-test modules use #[cfg_attr(coverage_nightly, coverage(off))] for test-harness code; their tests and assertions still execute, and their exercised production paths remain measured. The latest conflict resolution adds no coverage or mutation exclusions relative to current main.
Performables' standard Miri run after the earlier semantic merge passed all ten executables. The Seismograph regression previously passed standard, Tree Borrows and strict-provenance Miri; those full/profile runs were not repeated after the latest merge.
PR changed-line mutation testing after the earlier semantic merge passed: 22 mutants, 14 caught and 8 unviable, no misses or timeouts. The try_changed -> None replacement remains enabled and was caught. The latest merge leaves all PR-specific production changes unchanged; no fresh complete mutation campaign is claimed for this merge.
Prior Ubuntu selected library/channel/zero-allocation binaries passed 172 all-feature and 170 no-default tests. This is not a new full Ubuntu workspace run.
The earlier Linux Bolero memory-limit exit was investigated without code/limit changes: its recorded seed passed a full local empty-corpus campaign, and only the failed Linux job was retried. The Windows Miri retry was followed by the actual deterministic cleanup fix rather than treating a retry as a code correction.
✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (0c8f690) to head (676aeec). ⚠️ Report is 2 commits behind head on main.
Reviewed commit: b3d78d3b37dd34f5066e8a4ab3e3f0447a67587d
Base commit: 131f7d6a89a6dcef213179a9b5283a769d025a3c
Review time: 2026-10-13T06:19:48Z
I reviewed the public contract and API, correctness and concurrency, tests, performance and allocation behavior, naming, telemetry, resilience, source/documentation consistency, and integration tests. Generated README wording was excluded because it is repository-generated.
Focused all-feature tests, allocation guards, benchmark compilation, public API captures, and base/head integration tests passed. No base-versus-head timing or instruction-count comparison was run, so latency claims were not measured. Linux CI reports 99.9% coverage for its flag despite 100% patch coverage.
Public API changes
No API Changes
The complete all-feature performables 0.2.0 public surfaces at base and head are identical.
Integration tests
No Breaking Behavioral Changes
Existing behavior is preserved; tests add zero-timeout/final-version channel coverage and allocation-free ready-operation coverage.
Current findings
Posted by an AI agent · Non-blocking
Contended blocking sends lack a telemetry regression assertion
Problem
The telemetry test proves ready Sender::send calls emit no contention, but never exercises the new full-queue blocking branch or verifies that it emits exactly one ChannelSendContention event.
Why this matters
A future change could omit or duplicate contention telemetry while all current behavioral and allocation tests continue passing.
Suggested fix
Add a full bounded-queue scenario that blocks send, releases capacity, and asserts exactly one send-contention event.
Posted by an AI agent · Non-blocking
Barrier unit tests remain included in coverage accounting
Problem
New tests were added to barrier.rs without the repository's #[cfg_attr(coverage_nightly, coverage(off))] test-module convention, although the modified channel test module applies it.
Why this matters
Coverage reports mix test-harness execution with production coverage and become inconsistent across the changed modules.
Suggested fix
Apply #[cfg_attr(coverage_nightly, coverage(off))] to the barrier mod tests.
Posted by an AI agent · Nit
Private channel futures omit the established _async suffix
Problem
The new send_with_recording, recv_with_recording, and changed_with_recording functions are async, while every sibling async operation and the crate-level convention use an _async suffix.
Why this matters
Unsuffixed names imply blocking operations in this crate, obscuring whether callers must await or block_on them.
Suggested fix
Rename them to send_with_recording_async, recv_with_recording_async, and changed_with_recording_async.
Posted by an AI agent · Nit
Benchmark symbols omit parts of their reported operation names
Problem QUEUE_TIMEOUT/queue_timeout, ONESHOT_TIMEOUT/oneshot_timeout, and BARRIER_ASYNC/barrier_async diverge from their benchmark names queue_recv_timeout, oneshot_recv_timeout, and barrier_async_blocking. Sibling MUTEX_LOCK_PERFORMABLES_ASYNC_BLOCKING preserves the complete reported name.
Why this matters
Searching benchmark output does not lead directly to the corresponding source symbol, and barrier_async can be mistaken for benchmarking wait_async().
Suggested fix
Use QUEUE_RECV_TIMEOUT/queue_recv_timeout, ONESHOT_RECV_TIMEOUT/oneshot_recv_timeout, and BARRIER_ASYNC_BLOCKING/barrier_async_blocking.
Posted by an AI agent · Nit
The channel_allocations test binary now covers non-channel synchronization
Problem
The file describes ready blocking operations generally and includes a Barrier allocation test, while sibling channel.rs is channel-specific and broad synchronization tests use broader names such as sync.rs.
Why this matters
The test binary name no longer identifies its contents, making allocation failures for barriers appear under a channel-only target.
Suggested fix
Rename the file to ready_blocking_allocations.rs.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The stated coverage exclusions do not match the channel test-module exclusion in the diff.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Disclose or remove channel unit-test coverage exclusion
crates/performables/src/sync/channel.rs:1326
The validation notes say that only the barrier test harness was excluded from coverage, but this also disables coverage for the entire channel unit-test module, including the newly added pending-path tests. Please either remove this second exclusion or update the PR validation notes to disclose and explain it.
Add direct channel and async barrier fast paths while preserving shared waiting behavior, timeout deadlines, and contention telemetry. Guard ready-path allocation counts and pending-path behavior with deterministic tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e05ef33-3180-4057-9d92-e49459d4e7a8
Assert exactly one contention event for a full-queue blocking send, align private async and benchmark names, rename allocation guards, and clarify the async barrier blocking contract. Exclude only the barrier test harness from coverage accounting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e05ef33-3180-4057-9d92-e49459d4e7a8
Drain at least once before checking producer completion, retaining the original uniqueness, completeness, and tail-retention assertions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e05ef33-3180-4057-9d92-e49459d4e7a8
Exercise retired history explicitly, then verify active ring storage identity and capacity, retired ring release, exact recording policies, and retained events. Use fallible statistics so errors cannot appear as successful empty observations.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e05ef33-3180-4057-9d92-e49459d4e7a8
Resolve the worker-wait test conflict using main's fully joined private regressions and public cancellation coverage. Remove the superseded duplicate while preserving the optimization changes unchanged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0e05ef33-3180-4057-9d92-e49459d4e7a8
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The described Arty shutdown-test changes were absent from the supplied diff, leaving current-head review incomplete.
0 open findings
🧠 Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
martintmk
left a comment
Sander Saares (sandersaares)
left a comment
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Motivation
Blocking channel operations and async-capable barrier waits allocate a thread waker even when they can finish immediately. This adds avoidable allocation and latency to ready operations, unlike the existing uncontended mutex and reader-writer-lock fast paths.
Approach
ready_blocking_allocations.rs.No public API changes relative to current
main. Base integration retainsmain's crate-rootError,sync::{Async, Sync}, andBarrierWaitOutcomeAPIs from #820. Queue payload storage and enabled telemetry may allocate independently. Final async-capable barrier arrivals avoid allocating their own waiter storage; this does not promise allocation-free release of other waiters. Pending operations retain their existing blocking and cancellation behavior.Benchmark comparison
Compared against unmodified
mainat131f7d6a, with identical benchmark source, Rust 1.95, all features, fat LTO, and one logical CPU on Windows x64 / AMD Ryzen 7 PRO 7840U. Ran baseline/optimized/optimized/baseline, with 100 samples, 1-second warm-up and 2-second measurement per case. Values average the two independent-run medians. Before/after 95% confidence intervals remained separated across both repeats for every case.Measurements were taken at original commit
b3d78d3(replayed as6f4830e6), before later naming, documentation, test-only follow-ups, and base integrations. Reported benchmark identities and measured operations remain unchanged; synchronization type paths were adapted to #820. These timings were not rerun after the follow-ups, rebase, or merges.These are already-ready microbenchmarks, not contended throughput or end-to-end application speedups. Independent allocation tests reproduce one thread-waker allocation per ready call before the change and zero afterward, with queue payload storage prepared outside measurement.
Base integration and CI corrections
Current head
676aeeccmergesmainatf660d7c6. The sole textual conflict was in Arty's worker-wait regression, where #843 independently introduced a fuller deterministic-cleanup fix. The merge retains upstream's public cancellation test and private consuming-stop regressions, including all shutdown-completion joins, and removes this branch's superseded duplicate. Arty is now identical tomain; the PR is back to a 12-file diff. The optimization, allocator and Seismograph files are unchanged from the previously passing head627e1f59. The lockfile difference remains only theperformablesallocation-test dependency.Earlier integration preserved #820's diagnostics APIs/documentation and cargo-anvil 0.14.0 from #831. The generated crate README was regenerated, not edited by hand. The branch also includes the Quinn security fix from #816 and subsequent upstream lockfile updates; no advisory ignores were added.
Existing test defects were corrected without changing those crates' production behavior:
16f6f6f2(formerly5fb3b7dd): the allocator concurrent-publication test drains at least once before checking whether producers finished, so scheduling cannot skip its consumer loop. All original uniqueness, completeness, and tail-retention assertions remain.ec8c2569(formerly84acafd8): the Seismograph clear-buffer test distinguishes active allocation reuse from valid retired-thread cleanup. An explicit retired-thread fixture reproduced the old failure deterministically: total allocation fell from 37264 to 18832 bytes, the same 18432-byte difference seen in CI. The corrected test checks active ring allocation identity/capacity across enabled/disabled clear, exact policies and event counts, and actual retired-ring release. Fallible statistics prevent observation errors from appearing as empty results.62e0073b(formerly75fae2ab) gives the fixtures safely owned process-lifetime references, removing both new pointer dereferences reported by CodeQL without suppressing or dismissing alerts.The Windows Miri shutdown failure led to
627e1f59: nested runtime owners consumed on an async worker correctly request shutdown without waiting, so the regression needed retained completion handles and off-worker joins. That implementation is now superseded by the equivalent, broader upstream #843 tests, not dropped without replacement. No production runtime behavior or public API was changed by that follow-up.d7f12241(formerlya7b9fff0) addresses documentation/readability feedback with concise contracts, generated README updates, and worker-local handle ownership. All 14 threads from that review and both later CodeQL threads were replied to and resolved. Watchdogs and mutation cases remain enabled; no requested exclusions or watchdog disabling were introduced.Validation
Latest integration (
676aeecc)just anvil-clippypasses, as does explicit Rust 1.95 all-feature/all-target Clippy for arty, arty_executor, performables, rallocator and seismograph. The earlier local Spectre-library prerequisite blocker is no longer present in this validation.676aeecc; passing checks on627e1f59do not establish current-head success.Earlier measured evidence (not rerun on the latest merge)
Exact Windows source coverage was measured after the prior semantic merge at
a1d069ce, with zero raw uncovered LCOV source lines in both feature configurations:Arty was measured at 1563/1563 after
627e1f59; subsequent upstream lifecycle refactoring changes its source inventory, so that historical total is not a current Arty coverage claim. Linux coverage passed at 100% before the base integrations and must be reconfirmed by fresh CI. No fresh complete local Anvil PR-tier success is claimed.Both channel and barrier unit-test modules use
#[cfg_attr(coverage_nightly, coverage(off))]for test-harness code; their tests and assertions still execute, and their exercised production paths remain measured. The latest conflict resolution adds no coverage or mutation exclusions relative to currentmain.try_changed -> Nonereplacement remains enabled and was caught. The latest merge leaves all PR-specific production changes unchanged; no fresh complete mutation campaign is claimed for this merge.CI success does not imply approval or merge.