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(client, host): redundant state caching #59

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nhtyy
Copy link

@nhtyy nhtyy commented Oct 22, 2024

Background

Typically, CacheDb is used when reads from the underlying ExtDB are expensive. This cache represents the state at the beginning of the block, any state changes actually happen in a wrapper around the DB: Database type.

Internally CacheDB just stores queries in a memory map.

Client

In the client, CacheDB will make everything more expensive because the underlying witnessdb is really cheap to read from, so CacheDB does a clone that is not needed for every read.

Host

In the host, the RpcDB type does almost everything the CacheDB would do, this means. things like storage slots are saved twice in memory.

With minimal changes RpcDB we can replicate all the CacheDB behavior, and instead impl Database and we can pass a &mut of it to the executor.

Copy link
Contributor

@yuwen01 yuwen01 left a comment

Choose a reason for hiding this comment

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

I agree with your point on the witnessdb -- this is a really good catch that will probably save a lot of cycles. Left some small nits, also going to ask in the revm telegram any performance differences between database and databaseref. Because at this point, maybe we can just make WitnessDB a Database instead of DatabaseRef, but I'm not sure how good that is.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's an unused import in this file according to the linter

use reth_storage_errors::{db::DatabaseError, provider::ProviderError};
use revm_primitives::HashMap;

/// A database that fetches data from a [Provider] over a [Transport].
///
/// This type is very similar to a CacheDb as exposed by revm, except we have some extra fields
Copy link
Contributor

Choose a reason for hiding this comment

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

You can say that the caching part is similar, but the RpcDB actually uses an RPC to fetch the state, which is pretty different from the CacheDB which is just a wrapper.

@nhtyy
Copy link
Author

nhtyy commented Oct 23, 2024

adjusted comment + lint

@nhtyy
Copy link
Author

nhtyy commented Oct 23, 2024

Because at this point, maybe we can just make WitnessDB a Database instead of DatabaseRef, but I'm not sure how good that is.

I think compared to the current wrapper it'll save a deref

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.

2 participants