-
Notifications
You must be signed in to change notification settings - Fork 818
feat(blockdb): add lru cache for block entries #4425
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
base: master
Are you sure you want to change the base?
Conversation
af83e7d to
5e0ee3c
Compare
eb97db9 to
340fd4c
Compare
340fd4c to
c6240d3
Compare
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.
Pull Request Overview
This PR adds an LRU cache for block entries to blockdb to improve performance by avoiding repeated decompression of recently accessed block data.
- Implements an LRU cache with configurable size (defaults to 256 entries) for block data
- Updates cache on both Put and Get operations with cloned data to prevent modification
- Adds cache lookups to Get and Has operations to avoid disk access for cached entries
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| x/blockdb/config.go | Adds EntryCacheSize configuration field with default value and validation |
| x/blockdb/database.go | Implements LRU cache integration in Database struct and Get/Has/Put operations |
| x/blockdb/entry_cache_test.go | Comprehensive test suite for cache functionality including cloning behavior |
| x/blockdb/readblock_test.go | Updates test configurations to include new EntryCacheSize parameter |
| x/blockdb/writeblock_test.go | Fixes test assertion to handle nil values properly and adds bytes import |
| x/blockdb/README.md | Updates documentation to reflect cache feature and removes TODO item |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6064f29 to
20b70f1
Compare
ef7c706 to
afa7427
Compare
| // Operations (Get, Has, Put) are not atomic with the underlying database. | ||
| // Concurrent access to the same height can result in cache inconsistencies where | ||
| // the cache and database contain different values. This limitation is acceptable | ||
| // because concurrent writes to the same height are not an intended use case. |
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.
| // because concurrent writes to the same height are not an intended use case. | |
| // because concurrent access to the same height is not an intended use case. |
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.
concurrent Get/Has to the same height is fine and expected. its the writes to the same height that is not intended usage and will result in access inconsistencies from cached Has and Get
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.
Then it sounds wrong to me:
Concurrent access to the same height can result in cache inconsistencies [...] is acceptable because concurrent writes to the same height are not an intended use case.
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.
reverted to the original comment. 9792f38
I think both are right depending on how you interpret it but I think we can just call out the writes.
Why this should be merged
I noticed that during C-Chain block execution testing, each block accepted will trigger 3 block data fetches (header, body, and receipts). This happens when collecting unflattened logs in the acceptor queue (code).
Fetching block data should be fast but decompressing the data each time adds a lot of overhead. This overhead can be completely avoided with a cache because the data should have been just written to the database.
A cache was not originally added to blockdb because the VMs all have caches for blocks already. But they can be bypassed due to oversight and having a cache at the db level ensures these kind of issues are avoided.
How this works
PutandGet.GetandHasto avoid disk access.How this was tested
Unit tests and reexecution benchmark
Need to be documented in RELEASES.md?