Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(engine): migrate to AsyncStateRoot #10927

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

perf(engine): migrate to AsyncStateRoot #10927

wants to merge 14 commits into from
+40 −9

Conversation

fgimenez
Copy link
Member

Closes: #10892

Spawns the async calculation in a separate task and blocks waiting for the result, alternatively we could turn EngineApiTreeHandler::insert_block_inner and all its callers async.

@rkrasiuk
Copy link
Member

depends on #10920

@rkrasiuk rkrasiuk added C-perf A change motivated by improving speed, memory usage or disk footprint A-consensus Related to the consensus engine A-trie Related to Merkle Patricia Trie implementation labels Sep 16, 2024
@rkrasiuk rkrasiuk added the S-needs-benchmark This set of changes needs performance benchmarking to double-check that they help label Sep 16, 2024
@rkrasiuk
Copy link
Member

let's get some numbers in and then we can merge

@fgimenez fgimenez force-pushed the fgimenez/migrate-to-async-state-root branch from 090f3f9 to 4361936 Compare September 16, 2024 10:52
@fgimenez fgimenez force-pushed the fgimenez/migrate-to-async-state-root branch 2 times, most recently from d48d36f to 0a0b475 Compare September 17, 2024 07:53
rkrasiuk
rkrasiuk requested changes Sep 17, 2024
View reviewed changes
crates/engine/tree/src/tree/mod.rs Outdated Show resolved Hide resolved
rkrasiuk
rkrasiuk reviewed Sep 18, 2024
View reviewed changes
crates/engine/tree/src/tree/mod.rs Outdated
config: TreeConfig,
) -> Self {
let (incoming_tx, incoming) = std::sync::mpsc::channel();
let blocking_task_pool = BlockingTaskPool::build().unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's propagate the error here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm not sure, this would mean we can't build a rayon's thread pool, i've turned the unwrap into an expect like here https://github.com/paradigmxyz/reth/blob/main/crates/rpc/rpc/src/eth/core.rs#L101

@fgimenez fgimenez force-pushed the fgimenez/migrate-to-async-state-root branch 2 times, most recently from ec49301 to 1b111b4 Compare September 19, 2024 13:12
@fgimenez fgimenez force-pushed the fgimenez/migrate-to-async-state-root branch from 5602fdd to 0e37486 Compare September 19, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkrasiuk rkrasiuk rkrasiuk requested changes

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg

@rakita rakita Awaiting requested review from rakita rakita is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-consensus Related to the consensus engine A-trie Related to Merkle Patricia Trie implementation C-perf A change motivated by improving speed, memory usage or disk footprint S-needs-benchmark This set of changes needs performance benchmarking to double-check that they help
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

perf(engine): migrate to AsyncStateRoot
2 participants
@fgimenez @rkrasiuk