perf(tree): worker pooling for account proofs#18901
Conversation
- Added configuration for maximum and minimum storage proof workers. - Implemented a worker pool for processing storage proof tasks, improving efficiency by reusing transactions. - Updated `ProofTaskManager` to handle storage proof tasks via a dedicated channel. - Enhanced metrics to track storage proof requests and fallback scenarios. - Adjusted existing tests to accommodate the new storage worker functionality.
- Enhanced documentation for `StorageProofJob` to clarify its current unused status and potential for future type-safe design. - Updated comments in `ProofTaskManager` regarding the handling of on-demand tasks and the possibility of refactoring to a more type-safe enum. - Improved logging for worker pool disconnection scenarios, emphasizing fallback to on-demand execution.
…Metrics and ProofTaskTrieMetrics
…clarity - Updated comments in `ProofTaskManager` to enhance clarity regarding on-demand transaction handling and queue management. - Renamed `pending_on_demand` to `on_demand_queue` for better understanding of its purpose. - Adjusted the `new` function documentation to reflect the correct allocation of concurrency budget between storage workers and on-demand transactions. - Improved the `queue_proof_task` method to use the new queue name.
…ement - Removed the unused `OnDemandTask` enum and updated comments in `ProofTaskManager` to clarify the distinction between storage worker pool and on-demand execution. - Enhanced documentation to better describe the public interface and task submission process. - Improved clarity regarding transaction handling and execution paths for proof requests.
- Eliminated the `storage_proof_workers` field and related constants from `TreeConfig`. - Updated the default implementation and related methods to reflect the removal, streamlining the configuration structure.
- Improved comments in `ProofTaskManager` and related functions for better clarity on task management and processing. - Updated queue capacity calculation to use 4x buffering, reducing fallback to slower on-demand execution during burst loads. - Removed redundant variable assignments to streamline the code.
Co-authored-by: Brian Picciano <me@mediocregopher.com>
Co-authored-by: Brian Picciano <me@mediocregopher.com>
…ursor factories - Introduced pre-created cursor factories in `storage_worker_loop` to reduce overhead during proof computation. - Updated `compute_storage_proof` to accept cursor factories as parameters, enhancing efficiency and clarity. - Improved logging to provide better insights into proof task calculations.
- not change the logic for pending_tasks and proof_tasks_txs (on-demand proofs) and just continue using it for the BlindedAccountNode requests, but start using dedicated storage workers for StorageProof and BlindedStorageNode requests
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Added a function to determine the default number of storage worker threads based on available parallelism. - Updated TreeConfig to include a storage_worker_count field, initialized with the default value. - Modified payload processor to utilize the new storage_worker_count instead of a hardcoded value.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Rjected @shekhirin @mediocregopher ready for a final review 🙏🏻 |
| storage_proof_task_handle: ProofTaskManagerHandle, | ||
| /// Handle to the proof task manager used for account multiproofs. | ||
| account_proof_task_handle: ProofTaskManagerHandle, |
There was a problem hiding this comment.
this isaddressed later on in the pr for cleanup where we use proof_task_handle instead
| let storage_root_targets_len = StorageRootTargets::count( | ||
| &prefix_sets.account_prefix_set, | ||
| &prefix_sets.storage_prefix_sets, | ||
| ); |
There was a problem hiding this comment.
as a small optimization for future PRs, this calculation can be skipped if trace logs below won't be emitted
shekhirin
left a comment
There was a problem hiding this comment.
LGTM, my only suggestion is the same as Dan's — about free functions instead of structs with methods that keep the state.
Rjected
left a comment
There was a problem hiding this comment.
lgtm with the one suggestions about putting the worker state into structs
| } | ||
|
|
||
| impl<Factory> ProofTaskManager<Factory> | ||
| // TODO: Refactor this with storage_worker_loop. ProofTaskManager should be removed in the following |
There was a problem hiding this comment.
do you mean that we should keep the ProofTaskManager?
no, I just mean that a struct, and a method on the struct called run or something similar, makes more sense to me than having _loop functions, since they are tasks with state
ie something like
let worker = AccountProofWorker::new(...);
executor.spawn_blocking(move || {
worker.run();
});makes more sense to me than how we currently spawn
|
@shekhirin @Rjected will address Ur comments about structs in another pr |
* docs: yellowpaper sections in consensus implementation (paradigmxyz#18881) * fix(era-utils): fix off-by-one for Excluded end bound in process_iter (paradigmxyz#18731) Co-authored-by: Roman Hodulák <roman.hodulak@polyglot-software.com> * refactor: eliminate redundant allocation in precompile cache example (paradigmxyz#18886) * chore: relax `ChainSpec` impls (paradigmxyz#18894) * chore: make clippy happy (paradigmxyz#18900) * fix(trie): Reveal extension child when extension is last remaining child of a branch (paradigmxyz#18891) * chore(node): simplify EngineApiExt bounds by removing redundant constraints (paradigmxyz#18905) * refactor(engine): separate concerns in on_forkchoice_updated for better maintainability (paradigmxyz#18661) Co-authored-by: Nathaniel Bajo <nathanielbajo@Nathaniels-MacBook-Pro.local> Co-authored-by: YK <chiayongkang@hotmail.com> Co-authored-by: Brian Picciano <me@mediocregopher.com> * feat(provider): add get_account_before_block to ChangesetReader (paradigmxyz#18898) * refactor: replace collect().is_empty() with next().is_none() in tests (paradigmxyz#18902) Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> * ci: cache hive simulator images to reduce prepare-hive job time (paradigmxyz#18899) * docs: duplicate comment in Eip4844PoolTransactionError (paradigmxyz#18858) * chore: align node_config threshold constant (paradigmxyz#18914) * feat: wait for new blocks when build is in progress (paradigmxyz#18831) Co-authored-by: Roman Hodulák <roman.hodulak@polyglot-software.com> * perf(tree): worker pooling for storage in multiproof generation (paradigmxyz#18887) Co-authored-by: Brian Picciano <me@mediocregopher.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> * chore(grafana): use precompile address as legend (paradigmxyz#18913) * refactor: remove needless collect() calls in trie tests (paradigmxyz#18937) * fix(examples): change method to launch with debug capabilities (paradigmxyz#18946) * fix(example): launch with debug capabilities (paradigmxyz#18947) * fix(testsuite): Fix unused updates in e2e-test-utils (paradigmxyz#18953) * fix(payload): correct Debug label for PayloadTimestamp in PayloadServiceCommand (paradigmxyz#18954) * chore(rpc): Moves `SequencerMetrics` into `reth-optimism-rpc` (paradigmxyz#18921) * refactor: replace println! with structured logging in test_vectors (paradigmxyz#18956) * refactor(cli): use structured logging (tracing) in p2p command (paradigmxyz#18957) * perf(tree): add elapsed time to parallel state root completion log (paradigmxyz#18959) * fix(trie): Properly upsert into StoragesTrie in repair-trie (paradigmxyz#18941) * fix: misleading error message in db list: show actual table name (paradigmxyz#18896) * fix: remove noisy stderr prints in ERA1 cleanup (EraClient::delete_outside_range) (paradigmxyz#18895) * fix: use max B256 for upper bound in empty-storage check (paradigmxyz#18962) * ci: remove reproducible build from release.yml (paradigmxyz#18958) * chore(rpc): Remove redundant U256::from in suggested_priority_fee (paradigmxyz#18969) * chore(ci): update eest 7594 issue link in hive expected failures file (paradigmxyz#18976) * perf(tests): remove redundant format! in ef-tests run_only (paradigmxyz#18909) * feat(cli): enable traces export via `tracing-otlp` cli arg (paradigmxyz#18242) Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com> * feat: allow otlp level to be configurable (paradigmxyz#18981) * chore(optimism): remove unnecessary Debug bounds from header generics (paradigmxyz#18989) * refactor: convert satisfy_base_fee_ids to use closure (paradigmxyz#18979) * chore: bump otlp crates (paradigmxyz#18984) * fix(network): prevent metric leak in outgoing message queue on session teardown (paradigmxyz#18847) * chore: remove unused imports in blockchain_provider (paradigmxyz#18867) Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> * fix(stateless): enforce BLOCKHASH ancestor header limit (paradigmxyz#18920) * chore(evm): mark ExecuteOutput as unused and slated for removal (paradigmxyz#18754) * refactor: unify `Pipeline` creation codepaths (paradigmxyz#18955) * fix(engine): flatten storage cache (paradigmxyz#18880) * perf(tree): worker pooling for account proofs (paradigmxyz#18901) Co-authored-by: Brian Picciano <me@mediocregopher.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> * refactor(storage): fix ChainStateKey enum variant name (paradigmxyz#18992) * refactor(trie): remove proof task manager (paradigmxyz#18934) Co-authored-by: Brian Picciano <me@mediocregopher.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> * fix: required optimism primitives features in db-api (paradigmxyz#19005) * refactor(engine): simplify InvalidBlockWitnessHook::on_invalid_block for better testability (paradigmxyz#18696) * chore: replace poll_next_unpin loop with poll_recv_many (paradigmxyz#18978) * fix: Set Era pipeline stage to last checkpoint when there is no target (paradigmxyz#19000) * ci: Add tests for Paris scenario in hive.yml (paradigmxyz#19013) * chore: bump book timeout (paradigmxyz#19016) * feat: add metrics for safe and finalized block heights (paradigmxyz#18987) * chore(privitives-traits): remove unused serde derives and camelCase attribute (paradigmxyz#19014) * chore: refactor loop in `add_new_transactions` (paradigmxyz#19006) * chore(ci): bump hive eest to v5.3.0 (paradigmxyz#19021) * feat(devp2p): make eth p2p networkId configurable (paradigmxyz#19020) Co-authored-by: frankudoags <frankudoags.com> * chore: remove unused Args struct from exex-subscription example (paradigmxyz#19019) * feat: add pending sequence as pub (paradigmxyz#19022) * chore: bump alloy-core (paradigmxyz#19026) * fix: unused warnings for tracing (paradigmxyz#19025) * fix: respect cli blob size setting (paradigmxyz#19024) * feat(engine): deprecate TestPipelineBuilder::with_executor_results (paradigmxyz#19017) * perf: background init of workers (paradigmxyz#19012) * chore(ci): update expected failures (paradigmxyz#19034) * fix: use header type generic for mask (paradigmxyz#19037) * fix: correct `Compact` impl for `Option` (paradigmxyz#19042) * chore: increase versioned hash index cache (paradigmxyz#19038) * chore(primitives-traits): relax SignerRecoverable bounds for Extended<B,T> (paradigmxyz#19045) * feat: bump revm (paradigmxyz#18999) * fix: resolve upstream merge Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com> * bump revm * update deps and fix lints * update openvm deps * skip exex wal storage test * pin revm tag scroll-v91 * bump openvm compat --------- Signed-off-by: Gregory Edison <gregory.edison1993@gmail.com> Co-authored-by: josé v <52646071+Peponks9@users.noreply.github.com> Co-authored-by: Forostovec <ilonaforostovec22@gmail.com> Co-authored-by: Roman Hodulák <roman.hodulak@polyglot-software.com> Co-authored-by: Skylar Ray <137945430+sky-coderay@users.noreply.github.com> Co-authored-by: Arsenii Kulikov <klkvrr@gmail.com> Co-authored-by: Léa Narzis <78718413+lean-apple@users.noreply.github.com> Co-authored-by: Brian Picciano <me@mediocregopher.com> Co-authored-by: radik878 <radikpadik76@gmail.com> Co-authored-by: William Nwoke <willteey@gmail.com> Co-authored-by: Nathaniel Bajo <nathanielbajo@Nathaniels-MacBook-Pro.local> Co-authored-by: YK <chiayongkang@hotmail.com> Co-authored-by: Dan Cline <6798349+Rjected@users.noreply.github.com> Co-authored-by: Merkel Tranjes <140164174+rnkrtt@users.noreply.github.com> Co-authored-by: Alexey Shekhirin <5773434+shekhirin@users.noreply.github.com> Co-authored-by: Federico Gimenez <fgimenez@users.noreply.github.com> Co-authored-by: stevencartavia <112043913+stevencartavia@users.noreply.github.com> Co-authored-by: emmmm <155267286+eeemmmmmm@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: MIHAO PARK <wetkeyboard17@gmail.com> Co-authored-by: Tilak Madichetti <tilak.madichetti@gmail.com> Co-authored-by: Emilia Hane <elsaemiliaevahane@gmail.com> Co-authored-by: maradini77 <140460067+maradini77@users.noreply.github.com> Co-authored-by: sashaodessa <140454972+sashaodessa@users.noreply.github.com> Co-authored-by: Alvarez <140459501+prestoalvarez@users.noreply.github.com> Co-authored-by: MozirDmitriy <dmitriymozir@gmail.com> Co-authored-by: drhgencer <gancer16@gmail.com> Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de> Co-authored-by: anim001k <140460766+anim001k@users.noreply.github.com> Co-authored-by: Julian Meyer <julian.meyer@coinbase.com> Co-authored-by: Karl Yu <43113774+0xKarl98@users.noreply.github.com> Co-authored-by: Jennifer <jenpaff0@gmail.com> Co-authored-by: Ivan Wang <314130948@qq.com> Co-authored-by: GarmashAlex <garmasholeksii@gmail.com> Co-authored-by: Udoagwa Franklin <54338168+frankudoags@users.noreply.github.com> Co-authored-by: Luca Provini <luca.provini@usemerkle.com> Co-authored-by: Galoretka <galoretochka@gmail.com> Co-authored-by: sashass1315 <sashass1315@gmail.com> Co-authored-by: frisitano <francesco.risitano95@gmail.com>
This PR builds on top of #18887 by adding worker pooling to account and blinded account proofs, which allows us to reduce overhead from spawning the worker.
Changes:
Core logic:
Issues closed:
ProofTaskManagerfor account proofs fromMultiProofTask#18734ProofTaskManagerfor Blinded Node Migration #18833References:
Next Steps: