Skip to content

Conversation

yoavGrs
Copy link
Contributor

@yoavGrs yoavGrs commented Aug 28, 2025

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

yoavGrs commented Aug 28, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@yoavGrs yoavGrs self-assigned this Aug 28, 2025
@yoavGrs yoavGrs requested a review from TzahiTaub August 28, 2025 12:48
@yoavGrs yoavGrs marked this pull request as ready for review August 28, 2025 12:48
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from 41118e5 to daf27f9 Compare September 1, 2025 07:22
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/mdbx_playground branch from 62764e3 to 03ea0b4 Compare September 1, 2025 07:22
Copy link

github-actions bot commented Sep 1, 2025

Benchmark movements: No major performance changes detected.

@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from daf27f9 to 696af3c Compare September 7, 2025 06:57
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/mdbx_playground branch from 03ea0b4 to 90c575e Compare September 7, 2025 06:57
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from 696af3c to d911717 Compare September 15, 2025 08:43
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/mdbx_playground branch 2 times, most recently from d62797f to fd3ce7a Compare September 15, 2025 10:13
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from d911717 to 0b52a49 Compare September 15, 2025 13:16
@yoavGrs yoavGrs force-pushed the yoav/committer_benchmark/mdbx_playground branch from fd3ce7a to 91008e4 Compare September 15, 2025 13:16
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from 0b52a49 to 3ad2b8a Compare September 16, 2025 13:05
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 91008e4 to 68ada84 Compare September 16, 2025 13:05
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/add_benchmark_cli branch from 3ad2b8a to e37df54 Compare September 16, 2025 13:24
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 68ada84 to 94c835b Compare September 16, 2025 13:24
@TzahiTaub TzahiTaub changed the base branch from yoav/committer_benchmark/add_benchmark_cli to graphite-base/8917 September 17, 2025 09:45
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 94c835b to 2603923 Compare September 17, 2025 10:55
@TzahiTaub TzahiTaub changed the base branch from graphite-base/8917 to yoav/committer_benchmark/add_benchmark_cli September 17, 2025 10:55
@TzahiTaub TzahiTaub changed the base branch from yoav/committer_benchmark/add_benchmark_cli to graphite-base/8917 September 17, 2025 11:29
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 2603923 to 09b5dc6 Compare September 17, 2025 11:37
@TzahiTaub TzahiTaub changed the base branch from graphite-base/8917 to yoav/committer_benchmark/add_benchmark_cli September 17, 2025 11:37
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

@dorimedini-starkware reviewed 3 of 6 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)


crates/starknet_patricia_storage/src/mdbx_storage.rs line 21 at r5 (raw file):

                // LIFO policy for recycling a Garbage Collection items should be faster when using
                // disks with write-back cache.
                liforeclaim: true,

what's this...?

Code quote:

                // LIFO policy for recycling a Garbage Collection items should be faster when using
                // disks with write-back cache.
                liforeclaim: true,

crates/starknet_patricia_storage/src/mdbx_storage.rs line 43 at r5 (raw file):

        let table = txn.open_table(None)?;
        let prev_val = txn.get(&table, &key.0)?.map(DbValue);
        txn.put(&table, key.0, value.0, WriteFlags::UPSERT)?;

what's this?

Code quote:

WriteFlags::UPSERT

crates/starknet_patricia_storage/src/mdbx_storage.rs line 54 at r5 (raw file):

        for key in keys {
            res.push(txn.get(&table, &key.0)?.map(DbValue));
        }

libmdbx doesn't have a faster implementation of multi-read?

Code quote:

        for key in keys {
            res.push(txn.get(&table, &key.0)?.map(DbValue));
        }

crates/starknet_patricia_storage/src/mdbx_storage.rs line 63 at r5 (raw file):

        for (key, value) in key_to_value {
            txn.put(&table, key.0, value.0, WriteFlags::UPSERT)?;
        }

same Q (no faster impl?)

Code quote:

        for (key, value) in key_to_value {
            txn.put(&table, key.0, value.0, WriteFlags::UPSERT)?;
        }

crates/starknet_patricia_storage/src/mdbx_storage.rs line 75 at r5 (raw file):

            txn.del(&table, &key.0, None)?;
        }
        txn.commit()?;

is this required if prev_val is None?

Code quote:

txn.commit()?;

@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/add_benchmark_cli branch 2 times, most recently from 114e111 to fd1fbdd Compare September 17, 2025 14:38
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 09b5dc6 to b00a2ca Compare September 17, 2025 14:45
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

@dorimedini-starkware reviewed 16 of 16 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)

@TzahiTaub TzahiTaub changed the base branch from yoav/committer_benchmark/add_benchmark_cli to graphite-base/8917 September 18, 2025 07:51
@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from b00a2ca to 795c122 Compare October 5, 2025 12:07
@TzahiTaub TzahiTaub changed the base branch from graphite-base/8917 to committer_storage_benchmark October 5, 2025 12:07
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

@TzahiTaub reviewed 1 of 6 files at r4, 1 of 3 files at r5.
Reviewable status: 3 of 20 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs)


crates/starknet_patricia_storage/src/mdbx_storage.rs line 21 at r5 (raw file):

Previously, dorimedini-starkware wrote…

what's this...?

Added a TODO to go deeper into this later.


crates/starknet_patricia_storage/src/mdbx_storage.rs line 43 at r5 (raw file):

Previously, dorimedini-starkware wrote…

what's this?

UPdate if key exists, inSERT otherwise.


crates/starknet_patricia_storage/src/mdbx_storage.rs line 54 at r5 (raw file):

Previously, dorimedini-starkware wrote…

libmdbx doesn't have a faster implementation of multi-read?

Don't see one.


crates/starknet_patricia_storage/src/mdbx_storage.rs line 63 at r5 (raw file):

Previously, dorimedini-starkware wrote…

same Q (no faster impl?)

Don't see one.

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 20 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs)


crates/starknet_patricia_storage/src/mdbx_storage.rs line 75 at r5 (raw file):

Previously, dorimedini-starkware wrote…

is this required if prev_val is None?

Nope

@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from 795c122 to e2adee5 Compare October 5, 2025 12:18
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

@dorimedini-starkware reviewed 2 of 17 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: 6 of 20 files reviewed, all discussions resolved (waiting on @yoavGrs)


crates/starknet_patricia_storage/src/mdbx_storage.rs line 22 at r9 (raw file):

// TODO(tzahi): geometry and related definitions are taken from apollo_storage. Check if there are
// better configurations for the committer and consider moving boh crates mdbx code to a common
// location.

move this TODO to MdbxStorage::open please

Suggestion:

// TODO(tzahi): geometry, liforeclaim and related definitions are taken from apollo_storage. Check if there are
// better configurations for the committer and consider moving boh crates mdbx code to a common
// location.

crates/starknet_patricia_storage/src/mdbx_storage.rs line 26 at r9 (raw file):

// Size in bytes.
const MDBX_MIN_PAGESIZE: usize = 256;
const MDBX_MAX_PAGESIZE: usize = 65536; // 64KB

non-blocking, but if it works - it looks nicer

Suggestion:

const MDBX_MIN_PAGESIZE: usize = 1 << 8;
const MDBX_MAX_PAGESIZE: usize = 1 << 16; // 64KB

crates/starknet_patricia_storage/src/mdbx_storage.rs line 34 at r9 (raw file):

    if !page_size.is_power_of_two() {
        page_size = page_size.next_power_of_two() / 2;
    }

any reason to clamp again after this?
is it possible that MDBX_MIN_PAGESIZE will not be a power of two?

Code quote:

    // Page size must be power of two.
    if !page_size.is_power_of_two() {
        page_size = page_size.next_power_of_two() / 2;
    }

@TzahiTaub TzahiTaub force-pushed the yoav/committer_benchmark/mdbx_playground branch from e2adee5 to 3a6490f Compare October 5, 2025 13:15
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 of 20 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @yoavGrs)


crates/starknet_patricia_storage/src/mdbx_storage.rs line 34 at r9 (raw file):

Previously, dorimedini-starkware wrote…

any reason to clamp again after this?
is it possible that MDBX_MIN_PAGESIZE will not be a power of two?

This is a copy of the code in apollo_storage. I don't think it can be something else, but I wouldn't want to diverge from or change the code in apollo_storage in this PR.
And see (old answers here): https://stackoverflow.com/questions/40518454/is-it-safe-to-assume-memory-page-size-is-a-power-of-two
Seems that it would always be a power of 2 unless someone is making an exercise in a CS course.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@dorimedini-starkware reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: 6 of 20 files reviewed, all discussions resolved (waiting on @yoavGrs)

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

@dorimedini-starkware reviewed 14 of 17 files at r8.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

@TzahiTaub reviewed 1 of 6 files at r4, 3 of 3 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yoavGrs)

@TzahiTaub TzahiTaub merged commit fec4894 into committer_storage_benchmark Oct 5, 2025
34 of 36 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants