-
Notifications
You must be signed in to change notification settings - Fork 217
perf(l1): cut the cost of cold contract-code access #7095
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
Open
edg-l
wants to merge
11
commits into
main
Choose a base branch
from
perf/cold-contract-code-access
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
1cb8b7f
perf(l1): store jump destinations as a bitmap
edg-l c44a6af
perf(l1): bloom filter and 4KB blocks for the account-code CFs
edg-l 7ed03d9
perf(l1): answer EXTCODESIZE from the code-length table
edg-l a01c667
perf(l1): keep the panic path out of the JUMPDEST scan
edg-l 8b350e4
docs: changelog entry for cold contract-code access
edg-l 6c276f7
Merge remote-tracking branch 'origin/main' into perf/cold-contract-co…
edg-l 6dd6ca8
fix(l1): do not spawn the code-metadata backfill off-runtime
edg-l 3672421
fix(l1): address review on cold contract-code access
edg-l 8965902
Merge remote-tracking branch 'origin/main' into perf/cold-contract-co…
edg-l 9a4c1fd
fix(l1): keep derived jump destinations out of Code's wire format
edg-l bdb1f40
Merge remote-tracking branch 'origin/main' into pr7095
edg-l File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ pub type MigrationFn = fn(backend: &dyn StorageBackend) -> Result<(), StoreError | |
| /// | ||
| /// **Invariant**: `MIGRATIONS.len() == (STORE_SCHEMA_VERSION - 1) as usize` | ||
| /// (empty when `STORE_SCHEMA_VERSION == 1`, one entry when it's 2, etc.) | ||
| pub const MIGRATIONS: &[MigrationFn] = &[migrate_1_to_2, migrate_2_to_3]; | ||
| pub const MIGRATIONS: &[MigrationFn] = &[migrate_1_to_2, migrate_2_to_3, migrate_3_to_4]; | ||
|
|
||
| // Compile-time check: the number of migration functions must match the number | ||
| // of version gaps (i.e. STORE_SCHEMA_VERSION - 1). | ||
|
|
@@ -44,6 +44,19 @@ fn migration_for_version(version: u64) -> MigrationFn { | |
| MIGRATIONS[(version - 1) as usize] | ||
| } | ||
|
|
||
| /// v3 → v4: no data change. | ||
| /// | ||
| /// `ACCOUNT_CODES` values carry their JUMPDEST positions as a bitmap rather than an RLP | ||
| /// list of `u32` offsets. Both forms are readable, so a v3 database needs no rewriting | ||
| /// and this migration only moves the version marker. | ||
| /// | ||
| /// The bump exists for the other direction: a v3 binary cannot decode a bitmap, and | ||
| /// `run_pending_migrations` warns that the database is ahead of the binary instead of | ||
| /// letting it fail on the first code read. | ||
| fn migrate_3_to_4(_backend: &dyn StorageBackend) -> Result<(), StoreError> { | ||
| Ok(()) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there is no migration, the "recompute everything" fallback will be used on every cold read. This is likely going to cause a big performance hit. |
||
|
|
||
| /// Minimum interval between migration progress log lines. | ||
| const PROGRESS_LOG_INTERVAL: Duration = Duration::from_secs(10); | ||
|
|
||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the serialization of
Codeand therefore ofAccountUpdate, breakingstore_account_updates_by_block_number.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks. Fixed in 9a4c1fd by dropping the jump destinations from the wire format entirely and recomputing them on deserialize: they are a pure function of the bytecode, so carrying them coupled
AccountUpdates stored format to how they happen to be represented. The format is now hash plus bytecode, so a future representation change cannot break it again, and the payload loses a byte per eight bytes of code. Added a test asserting the field set.Note this still does not make rows written by an older binary readable, and the rollup store has no schema version to gate on. Since those rows are consumed at batch commit the window is small, but tell me if you want a version guard instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have a migration and not break things.
We could simply keep the field on CodeSerde and do the migration later if you want. For the encode path, transforming from a bitmap to a list should be easy.