Conversation
onbjerg
added
C-perf
The `DupWalker` crashed mdbx for some reason; possibly it drops something related to the internal MDBX cursor which I use later on.
onbjerg
force-pushed
the
onbjerg/executor-perf
branch
from
f5eb97c to
3a32d63
Compare
onbjerg
force-pushed
the
onbjerg/executor-perf
branch
from
edcd237 to
74368c3
Compare
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #1674 +/- ##
==========================================
+ Coverage 73.48% 73.69% +0.20%
==========================================
Files 405 405
Lines 49810 50035 +225
==========================================
+ Hits 36604 36872 +268
+ Misses 13206 13163 -43
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
onbjerg
changed the title
ExecutionResultExecutionResult
First one is implemented, second batch of comments are already tested, so the comments are obsolete.
onbjerg
force-pushed
the
onbjerg/executor-perf
branch
from
c5e0ae7 to
8944225
Compare
gakonst
left a comment
gakonst
left a comment
There was a problem hiding this comment.
LGTM. I love how this PR keys changesets by transition ID and lets us sort/split etc. more effectively than before.
I also like how we are clearly separating "insert block and all indexes" which are our append operations from "apply state changes" which is the more expensive part
| } | ||
|
|
||
| // Write storage changes | ||
| let mut storages_cursor = tx.cursor_dup_write::<tables::PlainStorageState>()?; |
There was a problem hiding this comment.
We can't, it is used in Change::StorageWiped to check if there is any storage to wipe at all
| if let Some((_, entry)) = storages_cursor.seek_exact(address)? { | ||
| storage_changeset_cursor.append_dup(storage_id, entry)?; | ||
|
|
||
| while let Some(entry) = storages_cursor.next_dup_val()? { |
There was a problem hiding this comment.
Excellent - cc @joshieDo on usage of the dup apis which you also made use of in the hashing/merkle stages and seem to be impactful
| let account_a = Account { balance: U256::from(1), nonce: 1, bytecode_hash: None }; | ||
| let account_b = Account { balance: U256::from(2), nonce: 2, bytecode_hash: None }; | ||
| let account_b_changed = Account { balance: U256::from(3), nonce: 3, bytecode_hash: None }; |
There was a problem hiding this comment.
nit we'll want to add some builder pattern funcs to the Account struct to make making these nicer, i.e. Account::default().set_balance(3 /* impl Into<U256>, may not be possible due to ruint perhaps? */).nonce(3);
| } | ||
| block_exec_res.finish_transition(); | ||
| next_transition_id += 1; | ||
| } |
There was a problem hiding this comment.
Should we log an error! if this was None?
There was a problem hiding this comment.
Don't think it is even possible that it is none as all transactions will always result in at least one change, but I am unsure. I just kept it in this structure to make it work for now, I will refactor it later (it was way too much to mentally keep track of in its current state)
| exec_res1.clone().write_to_db(tx.deref_mut(), 0).unwrap(); | ||
| tx.insert_block(block1.clone()).unwrap(); | ||
| tx.insert_hashes( |
There was a problem hiding this comment.
Main reaction here is that we used to call the function insert_block and now it only inserts the historical block indices, so we'll want to rename it into something more descriptive I think. And we may want to have a function around which does all 3 operations?
There was a problem hiding this comment.
We would never use the one that does all 3 in any part of main reth, maybe only in tests - but then again, what would you be testing in that case?
| entry.insert(code.clone()); | ||
| } | ||
| } | ||
| if !code.is_empty() && !db.contracts.contains_key(&account.info.code_hash) { |
There was a problem hiding this comment.
It is more efficent to have Entry, this is like double hashing code_hash as it needs two accesses to db.
6 participants
Restructures
ExecutionResultto be easier to write to the database in a more performant way.NODUPDATA,APPENDDUP,NEXTDUP)Still need to:
Some other things that might make sense to add: