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.

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

perf(provider): compute hashes and trie updates before opening write tx #5505

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

rkrasiuk
Copy link
Member

Description

Continuation of the freelist saga #5228

This PR aims to decrease the duration for which the write transaction is open during canonical commit. This is done by computing state hashes and merkle root with updates before opening a write transaction.

@rkrasiuk rkrasiuk added C-perf A change motivated by improving speed, memory usage or disk footprint A-db Related to the database labels Nov 20, 2023
@rkrasiuk rkrasiuk marked this pull request as ready for review November 21, 2023 16:42
@@ -2321,7 +2323,11 @@ impl<TX: DbTxMut + DbTx> BlockWriter for DatabaseProvider<TX> {
state.write_to_db(self.tx_ref(), OriginalValuesKnown::No)?;
durations_recorder.record_relative(metrics::Action::InsertState);

self.insert_hashes(first_number..=last_block_number, last_block_hash, expected_state_root)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's now insert_hashes inside the provider and new HashedStateChanges::write_to_db, can we merge them somehow? IIUC the provider one is used now only in the test

Copy link
Member Author

Choose a reason for hiding this comment

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

we might want to remove insert_hashes, will think about it


impl HashedStateChanges {
/// Write the bundle state to the database.
pub fn write_to_db<TX: DbTxMut + DbTx>(self, tx: &TX) -> Result<(), DatabaseError> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be nice to add the timings breakdown here too, same as in insert_hashes

Copy link
Collaborator

Choose a reason for hiding this comment

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

also a bit concerning that this is almost the same logic as in insert_(account|storage)_for_hashing

Copy link
Member Author

Choose a reason for hiding this comment

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

will address in a follow up

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, pending @shekhirin + some sanity testing

@rkrasiuk rkrasiuk force-pushed the rkrasiuk/compute-before-write branch from 31a72d7 to 25c8129 Compare November 28, 2023 07:42
@rkrasiuk rkrasiuk added this pull request to the merge queue Nov 28, 2023
@rkrasiuk rkrasiuk removed this pull request from the merge queue due to a manual request Nov 28, 2023
@rkrasiuk rkrasiuk enabled auto-merge November 28, 2023 08:43
@rkrasiuk rkrasiuk added this pull request to the merge queue Nov 28, 2023
Merged via the queue into main with commit 608f100 Nov 28, 2023
@rkrasiuk rkrasiuk deleted the rkrasiuk/compute-before-write branch November 28, 2023 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database C-perf A change motivated by improving speed, memory usage or disk footprint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants