Skip to content

Add LRU cache for eth block data and transactions statuses#479

Merged
sorpaas merged 4 commits intopolkadot-evm:masterfrom
moonbeam-foundation:jeremy-cache-logs
Sep 27, 2021
Merged

Add LRU cache for eth block data and transactions statuses#479
sorpaas merged 4 commits intopolkadot-evm:masterfrom
moonbeam-foundation:jeremy-cache-logs

Conversation

@nanocryk
Copy link
Copy Markdown
Contributor

When calling RPC (mainly related to logs) a lot of time is spent fetching the data from the database.
This PR adds an LRU cache to avoid reaching the database when many requests are related to the same block (this happen a lot with block explorers).

The cache use the substrate block hash as a key, as it won't change in case of reorg (instead of block id/height).

Followups / alternatives

  • Since these RPC apis are not async, this PR doesn't use async. When a block is not cached, it will fetch the database while not holding the lock, then obtain the lock only to insert the block data. It means while a block data is fetched other requests will also fetch the database. Only when one of them will put the data into the cache that subsequent requests will use the cache and not reach the DB. This is suboptimal but better than keeping the lock and preventing other blocks to be fetched/cached.
    If those RPC were converted to async a background task could manage the actual caching and access to the database, and would send the data back to the request handler through an async oneshot channel. If doing this async convertion is acceptable then I'll work on implementing it.
  • This cache have a limit in the number of blocks it keeps. However it doesn't look at their memory size, and the size of blocks can vary a lot. It may be better to put a memory limit to avoid exhausting resources in case of a lot of full blocks.

@nanocryk nanocryk requested a review from sorpaas as a code owner September 24, 2021 12:01
@cla-bot-2021
Copy link
Copy Markdown

cla-bot-2021 bot commented Sep 24, 2021

User @nanocryk, please sign the CLA here.

/// Storing them in an LRU cache will allow to reduce database accesses
/// when many subsequent requests are related to the same blocks.
pub struct EthBlockDataCache<B: BlockT> {
blocks: Mutex<LruCache<B::Hash, EthereumBlock>>,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please change all Mutex usage in this file to parking_lot::Mutex. Might also require you to change some unrelated code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also, isn't it better if we use RwLock here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see a few other std Mutex. If parking_lot is better, it would be a good follow-up PR to migrate all Mutex to parking_lot then ? I'll change only those new Mutex for the parking_lot version in this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you also change it to RwLock? Or what are the reasons not to?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since it's an LRU Cache get also take an &mut self since it'll bump the entry to the top of the cache.

@sorpaas
Copy link
Copy Markdown
Member

sorpaas commented Sep 27, 2021

Can you fix rustfmt?

@sorpaas sorpaas merged commit 0d8dccc into polkadot-evm:master Sep 27, 2021
@nanocryk nanocryk deleted the jeremy-cache-logs branch September 28, 2021 11:03
abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
…evm#479)

* add lru cache for eth block data and transactions statuses

* use parking_lot::Mutex

* cargo fmt
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