Repository navigation
Conversation
Migrate arty_executor task storage to plurality MultiPool raw handles and remove the legacy dependency, comparisons, documentation, and tooling references. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
There was a problem hiding this comment.
🟡 Changes recommended
The generated performance report now contains inaccurate claims about the surveyed pool implementations.
1 open finding
What changed in this PR
Migrates arty_executor task storage to plurality::MultiPool and removes infinity_pool.
Changes:
- Reworks raw task ownership and release through
plurality::Box. - Removes
infinity_pooldependencies and benchmarks. - Updates performance and design documentation.
| File | Description |
|---|---|
Cargo.toml |
Removes workspace dependency. |
Cargo.lock |
Updates resolved dependencies. |
crates/arty_executor/Cargo.toml |
Replaces executor dependency. |
crates/arty_executor/src/executor_core.rs |
Migrates task storage and reclamation. |
crates/arty_executor/src/task_ref.rs |
Implements raw pooled task references. |
crates/observed/benches/observed_benchmarks.rs |
Generalizes benchmark comments. |
crates/plurality/Cargo.toml |
Removes benchmark dependency. |
crates/plurality/benches/plurality.rs |
Removes comparison benchmarks. |
crates/plurality/benches/plurality_ops_common/mod.rs |
Removes comparison operations. |
crates/plurality/benches/pool_comparison/shared.rs |
Removes pool helpers. |
crates/plurality/docs/PERF.md |
Removes comparison results. |
crates/plurality/docs/design/multi-pool.md |
Removes design comparison. |
crates/plurality/scripts/perf_report.rs |
Updates report generation. |
crates/plurality/tests/alloc_tracking.rs |
Removes comparison allocation tests. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Clarify that comparison claims apply only to implementations retained in the generated report. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
There was a problem hiding this comment.
🟢 Approval recommended
The migration preserves the existing task lifecycle while consistently removing the obsolete dependency and related artifacts.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
martinhavelka (wukchung)
left a comment
martinhavelka (wukchung)
left a comment
There was a problem hiding this comment.
🤖 One verified finding on the migrated task storage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #803 +/- ##
=========================================
Coverage 100.0% 100.0%
=========================================
Files 769 796 +27
Lines 117689 119447 +1758
=========================================
+ Hits 117689 119447 +1758
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:
|
Avoid allocating test-only fake task references and restore owning-pool cleanup semantics when an executor is dropped during a panic or before shutdown completes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
Exercise dirty teardown across every scheduler collection and the pool-only state so mutation testing protects the plurality raw-handle cleanup path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
|
Release uninitialized and inert tasks during dirty teardown, but retain non-inert raw owners so surviving wakers cannot access freed wake signals. Cover both reclamation and retention under Miri and mutation testing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
Document that queued tasks are initialized but have never been polled, so their embedded wakers cannot have escaped. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
[Copilot speaking]
I reviewed the complete diff, public surface, raw task ownership and waker lifetime, dirty teardown behavior, tests, manifests, benchmarks, generated performance-report inputs, documentation, and affected callers. I found no remaining high-confidence issues; the plurality::MultiPool migration preserves exact-once reclamation while retaining non-inert tasks, and the dependency/comparison cleanup is consistent.
Verdict: approve.
|
re-run the gungraun and allocation benches |
Sander Saares (sandersaares)
left a comment
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 4 findings. No finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Miss |
Document the synthetic dirty-drop fixture boundary, distinguish pool routing from trait-object unsizing, and group plurality imports consistently. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
|
Posted by an AI agent Reviewed base: This complete review covered all eleven areas across all 16 changed files: public contracts, runtime correctness, tests, performance, naming, telemetry, resilience, consistency, public API, documentation, and integration tests. Changes remain necessary. Paired Linux Callgrind was not run. Exact-head-relevant Windows Criterion evidence was retained. A direct filtered Current findingsPosted by an AI agent Task-pool migration slows steady-state task churn Problem Why this matters Suggested fix Posted by an AI agent · Non-blocking The implementation performance guide still describes removed comparisons Problem Why this matters Suggested fix Posted by an AI agent · Non-blocking The executor's allocation-free claim omits debug diagnostics Problem Why this matters Suggested fix Posted by an AI agent · Nit The recovered waiter timeout is still named as a leak Problem Why this matters Suggested fix Posted by an AI agent · Non-blocking The shutdown-timeout test accepts an unrelated panic Problem Why this matters Suggested fix Posted by an AI agent · Non-blocking Dirty teardown has duplicate new-task coverage Problem Why this matters Suggested fix Public API changesNo API Changes All paired public API captures for Integration testsBreaking Behavioral Changes
Human review is required because established allocation contracts were removed. This monitor leaves labels unchanged. |
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Changes remain necessary. See the verified rolling review for the complete current results.
|
🤖 Agent-authored review [optional] Update the remaining heterogeneous-comparison documentationThe implementation performance guide still says the reference implementation's multi pools appear in the owning fat-pointer comparison and describes that comparison as the multi pool's cross-crate row. With those benchmarks removed, describe it instead as plurality's typed-versus-heterogeneous comparison with a standard-library allocator baseline. Evidence: Impact: The guide incorrectly implies that the remaining benchmarks compare plurality's heterogeneous pooling architecture against another pooling implementation. |
Adomas Bekeras (AdomasBekeras)
left a comment
Adomas Bekeras (AdomasBekeras)
left a comment
There was a problem hiding this comment.
🤖 Agent-authored approval, explicitly confirmed in Review Harness.
Skip task reclamation while already unwinding so panicking task destructors cannot trigger a process-aborting double panic. Add public-boundary allocation tracking that enforces diagnostic-only allocations in debug and zero allocations in release. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Changes remain necessary. See the verified rolling review for the complete current results.
Match existing same-crate drop-helper terminology and align the allocation test operation with the spawn-and-complete benchmark. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7563af05-5a5e-4e41-ac9e-8da71ac73baf
Co-authored-by: martintmk <103487740+martintmk@users.noreply.github.com>
martintmk
left a comment
martintmk
left a comment
There was a problem hiding this comment.
Posted by an AI agent
Changes remain necessary. See the verified rolling review for the complete current results.
| // panics or even memory safety violations in the test runner. This may break in the future, | ||
| // so be ready to adjust or remove as needed when additional complexity makes it impractical. | ||
| #[should_panic] | ||
| fn shutdown_times_out_with_leaked_waiter() { |
There was a problem hiding this comment.
Posted by an AI agent · Nit
The recovered waiter timeout is still named as a leak
Problem
Unlike sibling shutdown_times_out_with_leaked_join_handle, this test catches the timeout, drops the waiter, and completes shutdown. shutdown_times_out_with_leaked_waiter and leaked_waiter therefore describe the previous behavior.
Why this matters
The names obscure that this is recovery coverage rather than permanent dirty-shutdown coverage.
Suggested fix
Rename to shutdown_timeout_then_completes_after_waiter_release and retained_waiter.
|
|
||
| // We expect this to panic. | ||
| _ = executor.execute_cycle(); | ||
| assert!(std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| executor.execute_cycle())).is_err()); |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
The shutdown-timeout test accepts an unrelated panic
Problem
The is_err() assertion does not verify that execute_cycle() panicked specifically because the shutdown deadline expired.
Why this matters
Another panic that leaves the executor usable could satisfy the assertion, then allow waiter cleanup and the subsequent Shutdown, masking a timeout-path regression.
Suggested fix
Inspect the panic payload and assert "executor shutdown timed out" before dropping the waiter.
| } | ||
|
|
||
| #[test] | ||
| fn dirty_drop_reclaims_task_without_result_event() { |
There was a problem hiding this comment.
Posted by an AI agent · Non-blocking
Dirty teardown has duplicate new-task coverage
Problem
dirty_drop_reclaims_task_without_result_event duplicates dirty_drop_reclaims_new_task: both use TaskLocation::New, inert = true, and identical assertions.
Why this matters
The distinct name implies additional boundary coverage but exercises no additional behavior.
Suggested fix
Remove the duplicate test or restore a genuinely distinct fixture.
Summary
arty_executorheterogeneous task storage frominfinity_pool::RawBlindPooltoplurality::MultiPoolplurality::Box::into_raw/from_raw, including pinned access, cross-threadTaskReftransport, exact-once release, and steady-state slot reusePerformance
Measured the existing
arty_executormetabench target on isolatedmain(7a2de8ff) and this branch under WSL Callgrind/Gungraun. Instruction counts are deterministic on the same toolchain:Allocation-engine comparison remained at 0 allocations / 0 bytes for every warmed, sequential, and burst workload. First task registration changed from 10 allocations / 23,056 bytes to 11 allocations / 7,552 bytes: one additional cold layout-metadata allocation, but 67% fewer allocated bytes; the steady-state lifecycle remains allocation-free.
Validation
cargo check -p arty_executor -p plurality -p observedinfinity_pool/infinity-poolreferences remaingit diff --checkFull formatting/lint/test matrices are delegated to CI per the requested minimal-local-validation policy. The generated nightly Anvil formatter could not run locally because the pinned nightly lacks
rustfmt; directly changed packages were formatted with the installed stablerustfmtinstead.