refactor(lru): switch LRU implementation to golang-lru#3674
Conversation
Rationale is that LRU returns a error if the size is <= 0, and in our case, all the size parameters are comptime known constants, so discarding a error is fine at the moment, but panics (hopefully) protect us in the future in case of mis-use
There was a problem hiding this comment.
Pull request overview
This PR refactors the codebase’s LRU cache usage to rely on github.com/hashicorp/golang-lru/v2 (and simplelru) instead of the previous go-ethereum LRU helpers, centralizing construction logic in utils.
Changes:
- Added
utils.NewLRUandutils.NewSimpleLRUhelpers to standardize LRU construction. - Migrated RPC trace caches and pending pre-latest caching to HashiCorp LRU implementations.
- Updated aggregated bloom filter cache to use the new LRU and adjusted its constructor/tests accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/lru.go | Introduces shared constructors for thread-safe and non-thread-safe LRU caches. |
| sync/pending_polling.go | Switches the pre-latest “seen-by-parent” cache to simplelru via utils.NewSimpleLRU. |
| rpc/v8/handlers.go | Updates RPC v8 handler trace cache construction to use utils.NewLRU. |
| rpc/v9/handlers.go | Updates RPC v9 handler trace cache construction to use utils.NewLRU. |
| rpc/v10/handlers.go | Updates RPC v10 handler trace cache construction to use utils.NewLRU. |
| blockchain/aggregated_bloom_filter_cache.go | Replaces the previous LRU cache with HashiCorp’s cache and uses utils.NewLRU; changes constructor signature. |
| blockchain/blockchain.go | Adjusts blockchain initialization to match the aggregated bloom cache constructor change. |
| blockchain/aggregated_bloom_filter_cache_test.go | Updates tests to use the new aggregated bloom cache constructor and constant size. |
| go.mod | Adds github.com/hashicorp/golang-lru/v2 as a direct dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3674 +/- ##
==========================================
+ Coverage 76.16% 76.28% +0.12%
==========================================
Files 396 397 +1
Lines 36585 36560 -25
==========================================
+ Hits 27865 27890 +25
+ Misses 6743 6692 -51
- Partials 1977 1978 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rodrodros
left a comment
There was a problem hiding this comment.
Looks good, looking forward to see some benchmarks, and why SimpleLRU was used in someplaces. What about using 2 queues LRUs as well?
|
@rodrodros I've assumed that geth was using golang-lru under the hood, so I thought it would be 1-1 migration. I've double checked, and they use their own impl. I'll run the tests and post it here as a comment. |
There was a problem hiding this comment.
I am thinking it is better to create a package inside utils/lru/lru.go so that imports are called lru by default. We would wrap whatever implementation we used from there
|
tl;dr: golang-lru is ~20–35% slower per-op + 1 alloc per Add. Absolute deltas are ns-scale. While the implementation is slower, I think it's fine, since the callsites are not bottlenecked by the LRU performance, but rather by IO/network. Latency (ns/op)
Memory (B/op, allocs/op)
Summary
|
Replaces geth LRU implementation with golang-lru library. Adds a wrapper package, which exposes only API that are currently used.