chore(trie): Use Vec<Option<...>> in HashedPostStateCursors#19487
chore(trie): Use Vec<Option<...>> in HashedPostStateCursors#19487mediocregopher merged 16 commits intomainfrom
Conversation
github-actions
bot
added
A-trie
mediocregopher
changed the title
CodSpeed Performance ReportMerging #19487 will improve performances by 12.05%Comparing Summary
Benchmarks breakdown
|
|
Confirmed no impact on perf: |
mediocregopher
requested review from
Rjected,
joshieDo,
rakita and
shekhirin
as code owners
| } | ||
| updated_accounts.sort_unstable_by_key(|(address, _)| *address); | ||
| let accounts = HashedAccountsSorted { accounts: updated_accounts, destroyed_accounts }; | ||
| let mut accounts: Vec<_> = self.accounts.drain().collect(); |
There was a problem hiding this comment.
nice simplification - instead of filtering accounts into two separate collections, then sort the valid ones we just drain and sort
yongkangc
left a comment
yongkangc
left a comment
There was a problem hiding this comment.
lgtm overall, left some questions and nits
| pub accounts: HashedAccountsSorted, | ||
| /// Map of hashed addresses to hashed storage. | ||
| /// Sorted collection of account updates. `None` indicates a destroyed account. | ||
| pub accounts: Vec<(B256, Option<Account>)>, |
There was a problem hiding this comment.
would a type alias be nicer / more intuitive here?
There was a problem hiding this comment.
Imo it's ok as-is... but open to it if others thing it would be easier to read. Which part were you thinking would be better as an alias?
crates/trie/db/tests/post_state.rs
Outdated
| assert!(!cursor.is_storage_empty().unwrap()); | ||
| } | ||
|
|
||
| // all zero values, but not wiped |
There was a problem hiding this comment.
does this test catch implementations that would incorrectly treat zero values as empty storage?
There was a problem hiding this comment.
This is testing that given a single zero value in the overlay (which indicates the slot was deleted), is_storage_empty still returns false, since the db has other non-zero slots.
Co-authored-by: YK <chiayongkang@hotmail.com>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
mediocregopher
deleted the
mediocregopher/18848-0x00101010-rework-cursors
branch
3 participants
Fixes #18848
This continues the work done #19233. I've done a bit of cleanup and added more comprehensive proptests.
This PR modifies the in-memory representation of
HashedPostStateSortedso that it only uses a single Vec for both updates and deletions, rather than a Vec for updates and HashSet for deletions. This allows the type to more closely map to how changesets are stored in the database, and will make it simpler to serialize/deserialize this data in general (e.g. for #19430).Changing the in-memory representation of
HashedPostStateSortedrequires changing how the in-memory overlay for the database works. This was done in a manner essentially identical how InMemoryTrieCursor is implemented now, with a small complication that a trait is required to handle the two different value types of the hashed state tables (Account and U256).