[runtime] Support criterion::AsyncExecutor + rayon::ThreadPool#778
Conversation
| .filter_map(|x| self.notarizes[*x as usize].as_ref()); | ||
|
|
||
| // Recover threshold signature | ||
| let proposal_signature = self |
There was a problem hiding this comment.
Opted to remove this spawn code for now as data indicates there are much more efficient optimizations: #719
patrick-ogrady
changed the title
criterion::AsyncExecutorcriterion::AsyncExecutor + rayon::ThreadPool
| match msg { | ||
| Voter::Notarize(notarize) => { | ||
| // Notarize received digest | ||
| // Notarize random digest |
There was a problem hiding this comment.
Generally speaking, sending random first should cause more issues (as the first message is the one that ends up getting persisted).
| /// b.to_async(&executor).iter_batched(|| (), | ||
| /// |_| async { | ||
| /// // Get the context | ||
| /// let ctx = context::get::<commonware_runtime::tokio::Context>(); |
There was a problem hiding this comment.
Without control of the AsyncExecutor function interface, this is the best way IMHO to expose context to a benchmark (needed to setup storage, networking, etc.).
| where | ||
| F: Future + Send + 'static, | ||
| F::Output: Send + 'static; | ||
| F: Future; |
There was a problem hiding this comment.
This aligns us with tokio's interface
| /// | ||
| /// # Returns | ||
| /// A `Result` containing the configured [rayon::ThreadPool] or a [rayon::ThreadPoolBuildError] if the pool cannot be built. | ||
| pub fn create_pool<S: Spawner + Metrics>( |
There was a problem hiding this comment.
This is an unrelated change but needed for some of the cryptography cleanup work.
BrendanChou
left a comment
BrendanChou
left a comment
There was a problem hiding this comment.
Consensus stuff LGTM (less familiar with runtime)
There was a problem hiding this comment.
Pull Request Overview
This PR integrates support for criterion’s AsyncExecutor and rayon’s ThreadPool while addressing a regression from #735, along with several refactorings and updates in test logic throughout the runtime and consensus modules.
- Introduces a new helper function (create_pool) to create a rayon thread pool from a Spawner context.
- Removes unnecessary Send + 'static bounds from future trait constraints to enable more flexible async functionality.
- Adjusts consensus test behaviors and refactors task management in both deterministic and simplex runtime implementations.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| runtime/src/utils.rs | Added a helper to create a rayon-compatible thread pool and updated use statements. |
| runtime/src/tokio/runtime.rs | Removed Send constraints from futures to support more flexible execution. |
| runtime/src/deterministic.rs | Refactored task and queue management with new Tasks struct implementations. |
| runtime/src/benchmarks/* | Added a criterion-compatible executor for tokio. |
| consensus/src/** | Updated logic in tests and mocks to adjust consensus message handling and signature recovery. |
Comments suppressed due to low confidence (1)
consensus/src/threshold_simplex/actors/voter/actor.rs:363
- [nitpick] The variable used for the filtered iterator of notarizes is named ambiguously. Consider renaming it (for example, to filtered_notarizes) to improve clarity in the threshold signature recovery logic.
.filter_map(|x| self.notarizes[*x as usize].as_ref());
| let result = executor.start(future); | ||
|
|
||
| // Clean up | ||
| context::clear(); |
There was a problem hiding this comment.
It is recommended to use iter_custom to cleanup anything the runtime did (like storage) rather than using the executor to do so.
| }), | ||
| ); | ||
| // Pin root task to the heap | ||
| let mut root = Box::pin(f); |
There was a problem hiding this comment.
This change created the most complexity.
Now that f is neither Send nor 'static, we need to be a lot more careful about where it can be stored.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #778 +/- ##
==========================================
+ Coverage 89.37% 89.39% +0.01%
==========================================
Files 165 166 +1
Lines 41307 41136 -171
==========================================
- Hits 36917 36772 -145
+ Misses 4390 4364 -26
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Related: #727
The complexity here is that
criterionneeds to be able to create runtimes but the inner function needs to be able to get the context (not provided by thecriterion::block_onfunction).Example
Bug
Fixes a small regression introduced by #735
TODO
Wakerwith root task (otherwise sleep will skip ahead too far): [runtime] Fix root waker #791