feat: bytecode caching #82
Conversation
|
Claude finished @tac0turtle's task —— View job Code Review for Bytecode Caching PR
|
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the EVM execution environment by integrating sophisticated caching mechanisms. It introduces a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
tac0turtle
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two caching layers to improve EVM database performance: a BytecodeCache for immutable contract bytecode and a PinnedStorageCache for frequently accessed contract storage. The changes include the cache implementations, benchmarks to measure their impact, and integration into the node's payload builder.
My review focuses on the caching implementation in crates/ev-revm/src/cache.rs. I've identified two significant performance issues in the custom LruCache implementation:
- A
criticalissue where cache reads take a write lock, which will serialize all cache access and create a major concurrency bottleneck. - A
highseverity issue with the LRU eviction logic, which has O(N log N) complexity and will cause latency spikes.
I've recommended replacing the custom implementation with a specialized, production-ready caching crate like moka or lru to address these problems. I also have a medium severity comment on improving the code clarity in the new benchmark setup. The rest of the integration and configuration changes look good.
| /// | ||
| /// Returns `None` if the bytecode is not cached. | ||
| pub fn get(&self, code_hash: &B256) -> Option<Bytecode> { | ||
| let mut cache = self.cache.write().expect("cache lock poisoned"); |
There was a problem hiding this comment.
The get method acquires a write lock on the cache by calling self.cache.write(). This will become a major performance bottleneck under concurrent access, as it serializes all cache reads. The purpose of using an RwLock is to allow concurrent reads, but this implementation prevents that.
The write lock is necessary here because LruCache::get takes &mut self to update the access counter.
To fix this, I strongly recommend replacing the custom LruCache with a specialized concurrent LRU cache crate like moka or lru. These are designed for high-throughput, thread-safe caching and would solve this issue efficiently.
| // Collect entries sorted by access order (oldest first) | ||
| let mut entries: Vec<_> = self.entries.iter().map(|(k, (_, o))| (*k, *o)).collect(); | ||
| entries.sort_by_key(|(_, order)| *order); |
There was a problem hiding this comment.
The current LRU eviction strategy has a significant performance issue. It collects all cache entries into a vector and sorts them on every eviction. This is an O(N log N) operation, where N is the number of items in the cache. For a large cache (e.g., the default of 10,000 entries), this will cause noticeable latency spikes whenever the cache becomes full.
A more efficient LRU implementation would use a data structure like a doubly-linked list in conjunction with the HashMap to track usage order, allowing for O(1) eviction.
However, the best approach would be to use a well-vetted crate like lru or moka, which provide efficient and correct LRU cache implementations. This would also resolve the locking issue in BytecodeCache::get.
| for contract in &all_contracts { | ||
| mock_db = MockDatabase { | ||
| bytecodes: mock_db.bytecodes, | ||
| storage: { | ||
| let mut s = mock_db.storage; | ||
| for i in 0..slots_per_contract { | ||
| s.insert((*contract, U256::from(i)), U256::from(i * 1000)); | ||
| } | ||
| s | ||
| }, | ||
| latency_factor, | ||
| }; | ||
| } |
There was a problem hiding this comment.
This loop for populating the mock database storage is overly complex and inefficient. It reconstructs the MockDatabase struct on every iteration, which is hard to read and involves unnecessary moves of the bytecodes and storage hashmaps.
A cleaner approach would be to modify the with_storage method to take &mut self instead of self (perhaps renaming it to add_storage). This would allow you to simplify this block to a much more readable loop, for example:
for contract in &all_contracts {
mock_db.add_storage(*contract, slots_per_contract);
}
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes