Skip to content

Conversation

@nimrod-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@nimrod-starkware nimrod-starkware self-assigned this Jan 21, 2026
@nimrod-starkware nimrod-starkware marked this pull request as ready for review January 21, 2026 12:08
Copy link
Contributor Author

nimrod-starkware commented Jan 21, 2026

Copy link
Contributor

@ArielElp ArielElp left a comment

Choose a reason for hiding this comment

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

:lgtm:

@ArielElp reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware).


crates/starknet_patricia_storage/src/rocksdb_storage.rs line 60 at r1 (raw file):

        opts.set_write_buffer_size(WRITE_BUFFER_SIZE);
        opts.increase_parallelism(NUM_THREADS);
        opts.set_max_subcompactions(NUM_THREADS.try_into().unwrap());

(Non blocking) I thought you said this did not help much?

Also, are we sure about NUM_THREADS=MAX_BACKGROUND_JOBS=max_subcompactions? (worth verifying with cursor the numbers for 32 cores).

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 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware).


crates/starknet_patricia_storage/src/rocksdb_storage.rs line 70 at r2 (raw file):

        block.set_partition_filters(true);

        block.set_block_size(DB_BLOCK_SIZE);

what was this and why is it removed?

Code quote:

block.set_block_size(DB_BLOCK_SIZE);

crates/starknet_patricia_storage/src/rocksdb_storage.rs line 31 at r2 (raw file):

// memory).
const BLOOM_FILTER_NUM_BITS: f64 = 10.0;
const KEY_PREFIX_BYTES_LENGTH: usize = 32;

what is a "key prefix"?
our DB keys are always 64 bytes

Code quote:

const KEY_PREFIX_BYTES_LENGTH: usize = 32;

@nimrod-starkware nimrod-starkware changed the base branch from nimrod/no-wal to graphite-base/11874 January 22, 2026 13:28
@nimrod-starkware nimrod-starkware changed the base branch from graphite-base/11874 to main-v0.14.1-committer January 22, 2026 13:28
Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

@nimrod-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware).


crates/starknet_patricia_storage/src/rocksdb_storage.rs line 60 at r1 (raw file):

Previously, ArielElp wrote…

(Non blocking) I thought you said this did not help much?

Also, are we sure about NUM_THREADS=MAX_BACKGROUND_JOBS=max_subcompactions? (worth verifying with cursor the numbers for 32 cores).

I expected it to improve the 1k-constant flavor, but it didn't make it better or worst.
Cursor suggested to use it and set it to the number of cores the machine has.
Maybe the constant NUM_THREADS should be given from the committer config, @dorimedini-starkware wdyt?

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 made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @nimrod-starkware).


crates/starknet_patricia_storage/src/rocksdb_storage.rs line 60 at r1 (raw file):

Previously, nimrod-starkware wrote…

I expected it to improve the 1k-constant flavor, but it didn't make it better or worst.
Cursor suggested to use it and set it to the number of cores the machine has.
Maybe the constant NUM_THREADS should be given from the committer config, @dorimedini-starkware wdyt?

should be configurable, yes, as should (many?) other fields.
can one of you take this (in a separate PR)?

Copy link
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

@nimrod-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp and @dorimedini-starkware).


crates/starknet_patricia_storage/src/rocksdb_storage.rs line 60 at r1 (raw file):

Previously, dorimedini-starkware wrote…

should be configurable, yes, as should (many?) other fields.
can one of you take this (in a separate PR)?

I can take it, added a todo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants