perf: warm transactions in parallel #13759
Conversation
Rjected
added
C-enhancement
crates/engine/tree/src/tree/mod.rs
Outdated
| ); | ||
|
|
||
| // TODO: move this logic somewhere better? | ||
| for tx in block.body().transactions() { |
There was a problem hiding this comment.
you could wrap the entire thing in pool.scope(|s| ) to avoid cloning but then it would block at scope end
There was a problem hiding this comment.
scoping could be useful because we will need to make sure all tasks are complete when we are doing cross-block caching + prewarming (basically using scope as a waitgroup). But I think it would make this fn body a bit of a mess, and we could accomplish a similar thing by using a RwLock
Rjected
force-pushed
the
dan/parallel-warming
branch
11 times, most recently
from
0d6178c to
97fd66a
Compare
Rjected
force-pushed
the
dan/parallel-warming
branch
14 times, most recently
from
034e958 to
1c3d657
Compare
Rjected
requested review from
onbjerg,
klkvr,
rkrasiuk,
fgimenez and
gakonst
as code owners
Rjected
force-pushed
the
dan/parallel-warming
branch
3 times, most recently
from
8310175 to
404b30e
Compare
crates/engine/local/src/service.rs
Outdated
There was a problem hiding this comment.
fyi @klkvr likely conflicts with revm integration wip
crates/engine/tree/src/tree/mod.rs
Outdated
| }; | ||
|
|
||
| // if execution is finished there is no point to sending proof targets | ||
| if execution_finished.load(std::sync::atomic::Ordering::SeqCst) { |
There was a problem hiding this comment.
we have this a few times,
we could duplicate this
reth/crates/payload/basic/src/lib.rs
Line 688 in bf20c78
or import if in scope or move to somewhere shared
There was a problem hiding this comment.
yeah adding somewhere else and sharing makes sense
There was a problem hiding this comment.
moved to reth_revm, now reused in both places
crates/engine/tree/src/tree/mod.rs
Outdated
| // the block leads back to the canonical chain | ||
| let historical = self.provider.state_by_block_hash(historical)?; | ||
| return Ok(Some(Box::new(MemoryOverlayStateProvider::new(historical, blocks)))) | ||
| } | ||
|
|
||
| // the hash could belong to an unknown block or a persisted block | ||
| if let Some(header) = self.provider.header(&hash)? { | ||
| debug!(target: "engine::tree", %hash, number = %header.number(), "found canonical state for block in database"); | ||
| debug!(target: "engine::tree", ?hash, number = ?header.number(), "found canonical state for block in database"); |
There was a problem hiding this comment.
Why change to Debug? you should generally always prefer Display (%)
There was a problem hiding this comment.
whoops, artifact from when i removed these logs since they were weirdly showing up as taking a lot of time in some profiles
crates/engine/tree/src/tree/mod.rs
Outdated
| let ResultAndState { state, .. } = match evm.transact(tx_env) { | ||
| Ok(res) => res, | ||
| Err(err) => { | ||
| error!(target: "engine::tree", ?err, tx_hash=?tx.tx_hash(), ?sender, "Error when executing prewarm transaction"); |
There was a problem hiding this comment.
isn't error! too verbose here? also same as last comment
There was a problem hiding this comment.
yeah, will change to trace or debug
Rjected
force-pushed
the
dan/parallel-warming
branch
2 times, most recently
from
0cc94b1 to
41774d3
Compare
Rjected
force-pushed
the
dan/parallel-warming
branch
from
41774d3 to
8dc62b5
Compare
Rjected
force-pushed
the
dan/parallel-warming
branch
from
8dc62b5 to
4d0345e
Compare
Rjected
force-pushed
the
dan/parallel-warming
branch
2 times, most recently
from
08000b7 to
eacc83a
Compare
Rjected
force-pushed
the
dan/parallel-warming
branch
from
eacc83a to
af9a8a6
Compare
This spawns tx prewarm threads on a dedicated threadpool, similar to the state root task threadpool
ref #13713