Skip to content

Conversation

@Rjected
Copy link
Member

@Rjected Rjected commented Oct 6, 2025

This fixes memory growth caused by growing execution caches, an alternative to #18879

This instead removes the cache hierarchy, so cache updates are always picked up by the moka weigher.

Previous memory usage:
Screenshot 2025-10-03 at 3 29 31 PM

New memory usage:
Screenshot 2025-10-03 at 5 07 15 PM

This also has slightly higher cache hitrate than #18879

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.

makes sense, let's go with this

I'm a bit concerned about the invalidate impl

/// storage slots.
storage_cache: Cache<Address, AccountStorageCache>,
/// Flattened storage cache: composite key of (`Address`, `StorageKey`) maps directly to values.
storage_cache: Cache<(Address, StorageKey), Option<StorageValue>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

right, we can't really nest caches because this would mean that the weigher of the storage_cache would only consider new acc -> storage entries

this is better because it treats storage slots as individual entries, which I guess explains the higher hit rate

we likely have additional address overhead, but this should be okay, we could compress this to a u64 with a separate map, like we do in the pool, but perhaps not that useful

Comment on lines 357 to 363
let storage_entries = self
.storage_cache
.iter()
.filter_map(|entry| addresses.contains(&entry.key().0).then_some(*entry.key()));
for key in storage_entries {
self.storage_cache.invalidate(&key)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, unsure this is sound because this feels problematic

dashmap iter

Locking behaviour: May deadlock if called when holding a mutable reference into the map.

and cache iter:

Locking behavior
This iterator relies on the iterator of dashmap::DashMap , which employs read-write locks. May deadlock if the thread holding an iterator attempts to update the cache.

tho I don't know if this is applicable here
unfortunately we don't have a retain here

should be perhaps collect then remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, yeah I think we should collect then remove here

Copy link
Member Author

Choose a reason for hiding this comment

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

this now collects

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should document this with a one liner because this is a prime candidate for a bot "optimization" pr that would look perfectly valid

@github-project-automation github-project-automation bot moved this from Backlog to In Progress in Reth Tracker Oct 14, 2025
@Rjected Rjected force-pushed the dan/flatten-storage-cache branch from 0e36a74 to eb8a43a Compare October 14, 2025 18:14
@Rjected Rjected force-pushed the dan/flatten-storage-cache branch from eb8a43a to 0f83128 Compare October 14, 2025 18:17
@Rjected Rjected requested a review from mattsse October 14, 2025 18:17
@Rjected Rjected marked this pull request as ready for review October 14, 2025 18:20
Comment on lines 357 to 363
let storage_entries = self
.storage_cache
.iter()
.filter_map(|entry| addresses.contains(&entry.key().0).then_some(*entry.key()));
for key in storage_entries {
self.storage_cache.invalidate(&key)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should document this with a one liner because this is a prime candidate for a bot "optimization" pr that would look perfectly valid

@Rjected Rjected enabled auto-merge October 14, 2025 19:01
@Rjected Rjected added this pull request to the merge queue Oct 14, 2025
Merged via the queue into main with commit 169a1fb Oct 14, 2025
39 of 40 checks passed
@Rjected Rjected deleted the dan/flatten-storage-cache branch October 14, 2025 19:28
@github-project-automation github-project-automation bot moved this from In Progress to Done in Reth Tracker Oct 14, 2025
@jenpaff jenpaff added this to the v1.8.3 milestone Oct 16, 2025
@jenpaff jenpaff mentioned this pull request Oct 16, 2025
1 task
Rjected added a commit that referenced this pull request Oct 22, 2025
@jenpaff jenpaff moved this from Done to Completed in Reth Tracker Oct 30, 2025
shekhirin added a commit that referenced this pull request Oct 30, 2025
shekhirin added a commit that referenced this pull request Oct 31, 2025
frisitano added a commit to scroll-tech/reth that referenced this pull request Nov 17, 2025
* docs: yellowpaper sections in consensus implementation (paradigmxyz#18881)

* fix(era-utils): fix off-by-one for Excluded end bound in process_iter (paradigmxyz#18731)

Co-authored-by: Roman Hodulák <[email protected]>

* refactor: eliminate redundant allocation in precompile cache example (paradigmxyz#18886)

* chore: relax `ChainSpec` impls (paradigmxyz#18894)

* chore: make clippy happy (paradigmxyz#18900)

* fix(trie): Reveal extension child when extension is last remaining child of a branch (paradigmxyz#18891)

* chore(node): simplify EngineApiExt bounds by removing redundant constraints (paradigmxyz#18905)

* refactor(engine): separate concerns in on_forkchoice_updated for better maintainability (paradigmxyz#18661)

Co-authored-by: Nathaniel Bajo <[email protected]>
Co-authored-by: YK <[email protected]>
Co-authored-by: Brian Picciano <[email protected]>

* feat(provider): add get_account_before_block to ChangesetReader (paradigmxyz#18898)

* refactor: replace collect().is_empty() with next().is_none() in tests (paradigmxyz#18902)

Co-authored-by: Alexey Shekhirin <[email protected]>

* ci: cache hive simulator images to reduce prepare-hive job time (paradigmxyz#18899)

* docs: duplicate comment in Eip4844PoolTransactionError (paradigmxyz#18858)

* chore: align node_config threshold constant (paradigmxyz#18914)

* feat: wait for new blocks when build is in progress (paradigmxyz#18831)

Co-authored-by: Roman Hodulák <[email protected]>

* perf(tree): worker pooling for storage in multiproof generation (paradigmxyz#18887)

Co-authored-by: Brian Picciano <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Alexey Shekhirin <[email protected]>

* chore(grafana): use precompile address as legend (paradigmxyz#18913)

* refactor: remove needless collect() calls in trie tests (paradigmxyz#18937)

* fix(examples): change method to launch with debug capabilities (paradigmxyz#18946)

* fix(example): launch with debug capabilities (paradigmxyz#18947)

* fix(testsuite): Fix unused updates in e2e-test-utils (paradigmxyz#18953)

* fix(payload): correct Debug label for PayloadTimestamp in PayloadServiceCommand (paradigmxyz#18954)

* chore(rpc): Moves `SequencerMetrics` into `reth-optimism-rpc` (paradigmxyz#18921)

* refactor: replace println! with structured logging in test_vectors (paradigmxyz#18956)

* refactor(cli): use structured logging (tracing) in p2p command (paradigmxyz#18957)

* perf(tree): add elapsed time to parallel state root completion log (paradigmxyz#18959)

* fix(trie): Properly upsert into StoragesTrie in repair-trie (paradigmxyz#18941)

* fix: misleading error message in db list: show actual table name (paradigmxyz#18896)

* fix: remove noisy stderr prints in ERA1 cleanup (EraClient::delete_outside_range) (paradigmxyz#18895)

* fix: use max B256 for upper bound in empty-storage check (paradigmxyz#18962)

* ci: remove reproducible build from release.yml (paradigmxyz#18958)

* chore(rpc): Remove redundant U256::from in suggested_priority_fee (paradigmxyz#18969)

* chore(ci): update eest 7594 issue link in hive expected failures file (paradigmxyz#18976)

* perf(tests): remove redundant format! in ef-tests run_only (paradigmxyz#18909)

* feat(cli): enable traces export via `tracing-otlp` cli arg (paradigmxyz#18242)

Co-authored-by: Dan Cline <[email protected]>

* feat: allow otlp level to be configurable (paradigmxyz#18981)

* chore(optimism): remove unnecessary Debug bounds from header generics (paradigmxyz#18989)

* refactor: convert satisfy_base_fee_ids to use closure (paradigmxyz#18979)

* chore: bump otlp crates (paradigmxyz#18984)

* fix(network): prevent metric leak in outgoing message queue on session teardown (paradigmxyz#18847)

* chore: remove unused imports in blockchain_provider (paradigmxyz#18867)

Co-authored-by: Matthias Seitz <[email protected]>

* fix(stateless): enforce BLOCKHASH ancestor header limit (paradigmxyz#18920)

* chore(evm): mark ExecuteOutput as unused and slated for removal (paradigmxyz#18754)

* refactor: unify `Pipeline` creation codepaths (paradigmxyz#18955)

* fix(engine): flatten storage cache (paradigmxyz#18880)

* perf(tree): worker pooling for account proofs (paradigmxyz#18901)

Co-authored-by: Brian Picciano <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Alexey Shekhirin <[email protected]>

* refactor(storage): fix ChainStateKey enum variant name (paradigmxyz#18992)

* refactor(trie): remove proof task manager (paradigmxyz#18934)

Co-authored-by: Brian Picciano <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Alexey Shekhirin <[email protected]>

* fix: required optimism primitives features in db-api (paradigmxyz#19005)

* refactor(engine): simplify InvalidBlockWitnessHook::on_invalid_block for better testability (paradigmxyz#18696)

* chore: replace poll_next_unpin loop with poll_recv_many (paradigmxyz#18978)

* fix: Set Era pipeline stage to last checkpoint when there is no target (paradigmxyz#19000)

* ci: Add tests for Paris scenario in hive.yml (paradigmxyz#19013)

* chore: bump book timeout (paradigmxyz#19016)

* feat: add metrics for safe and finalized block heights (paradigmxyz#18987)

* chore(privitives-traits): remove unused serde derives and camelCase attribute (paradigmxyz#19014)

* chore: refactor loop in `add_new_transactions` (paradigmxyz#19006)

* chore(ci): bump hive eest to v5.3.0 (paradigmxyz#19021)

* feat(devp2p): make eth p2p networkId configurable (paradigmxyz#19020)

Co-authored-by: frankudoags <frankudoags.com>

* chore: remove unused Args struct from exex-subscription example (paradigmxyz#19019)

* feat: add pending sequence as pub (paradigmxyz#19022)

* chore: bump alloy-core (paradigmxyz#19026)

* fix: unused warnings for tracing (paradigmxyz#19025)

* fix: respect cli blob size setting (paradigmxyz#19024)

* feat(engine): deprecate TestPipelineBuilder::with_executor_results (paradigmxyz#19017)

* perf: background init of workers (paradigmxyz#19012)

* chore(ci): update expected failures (paradigmxyz#19034)

* fix: use header type generic for mask (paradigmxyz#19037)

* fix: correct `Compact` impl for `Option` (paradigmxyz#19042)

* chore: increase versioned hash index cache (paradigmxyz#19038)

* chore(primitives-traits): relax SignerRecoverable bounds for Extended<B,T> (paradigmxyz#19045)

* feat: bump revm (paradigmxyz#18999)

* fix: resolve upstream merge

Signed-off-by: Gregory Edison <[email protected]>

* bump revm

* update deps and fix lints

* update openvm deps

* skip exex wal storage test

* pin revm tag scroll-v91

* bump openvm compat

---------

Signed-off-by: Gregory Edison <[email protected]>
Co-authored-by: josé v <[email protected]>
Co-authored-by: Forostovec <[email protected]>
Co-authored-by: Roman Hodulák <[email protected]>
Co-authored-by: Skylar Ray <[email protected]>
Co-authored-by: Arsenii Kulikov <[email protected]>
Co-authored-by: Léa Narzis <[email protected]>
Co-authored-by: Brian Picciano <[email protected]>
Co-authored-by: radik878 <[email protected]>
Co-authored-by: William Nwoke <[email protected]>
Co-authored-by: Nathaniel Bajo <[email protected]>
Co-authored-by: YK <[email protected]>
Co-authored-by: Dan Cline <[email protected]>
Co-authored-by: Merkel Tranjes <[email protected]>
Co-authored-by: Alexey Shekhirin <[email protected]>
Co-authored-by: Federico Gimenez <[email protected]>
Co-authored-by: stevencartavia <[email protected]>
Co-authored-by: emmmm <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: MIHAO PARK <[email protected]>
Co-authored-by: Tilak Madichetti <[email protected]>
Co-authored-by: Emilia Hane <[email protected]>
Co-authored-by: maradini77 <[email protected]>
Co-authored-by: sashaodessa <[email protected]>
Co-authored-by: Alvarez <[email protected]>
Co-authored-by: MozirDmitriy <[email protected]>
Co-authored-by: drhgencer <[email protected]>
Co-authored-by: Matthias Seitz <[email protected]>
Co-authored-by: anim001k <[email protected]>
Co-authored-by: Julian Meyer <[email protected]>
Co-authored-by: Karl Yu <[email protected]>
Co-authored-by: Jennifer <[email protected]>
Co-authored-by: Ivan Wang <[email protected]>
Co-authored-by: GarmashAlex <[email protected]>
Co-authored-by: Udoagwa Franklin <[email protected]>
Co-authored-by: Luca Provini <[email protected]>
Co-authored-by: Galoretka <[email protected]>
Co-authored-by: sashass1315 <[email protected]>
Co-authored-by: frisitano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Completed

Development

Successfully merging this pull request may close these issues.

4 participants