feat(rpc): add neutral trace cache primitives - #4065
danielntmd wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4065 +/- ##
==========================================
+ Coverage 79.19% 79.38% +0.18%
==========================================
Files 464 469 +5
Lines 35741 36095 +354
==========================================
+ Hits 28305 28653 +348
- Misses 7427 7433 +6
Partials 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review in progress
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
rodrodros
left a comment
rodrodros
left a comment
There was a problem hiding this comment.
Overall, it looks good, some nitpicks here and there.
I will need to read the follow up PR to see the cache in action and give a proper review to this one.
| return nil, errors.New("VM returned an unexpected number of transaction traces") | ||
| } | ||
| if len(result.GasConsumed) != len(result.Traces) { | ||
| return nil, errors.New("VM returned an unexpected number of gas results") |
There was a problem hiding this comment.
Please add expected and received info to the errors
| func (w *Lease[K, V]) Publish(value V) { | ||
| cache := w.cache | ||
| cache.mu.Lock() | ||
| defer cache.mu.Unlock() | ||
| if cache.flights[w.key] != w.flight { | ||
| return | ||
| } | ||
| cache.records.Add(w.key, value) | ||
| cache.finishLocked(&w.key, w.flight) | ||
| } | ||
|
|
||
| // Abort releases ownership without changing the value. Repeated calls are safe. | ||
| func (w *Lease[K, V]) Abort() { |
There was a problem hiding this comment.
nit: maybe use l for Lease instead of w?
| @@ -0,0 +1,174 @@ | |||
| package tracecache | |||
There was a problem hiding this comment.
This test file have unit tests checking your implementation rather than the expected behaviour. Is best when writing unit test to fully focus on behaviour only. A close translation of behaviour testing only is limiting your unit tests to only your public API.
Luckily, Golang also helps with this, if you rename tracecache to tracecache_test the compiler will hide private API from you.
There is the added benefit that if tomorrow we rewrite the whole functionality and maintain public API tests will need no maintenance and it will be easier to test behaviour is maintained.
Common pitfall I've seen on other devs when trying to surpass this requirements is making private API public for the sole purpose of testing, just noting here to try to avoid.
To sum up, action items:
- rename
tracecachetotracecache_test - update the functions to test behaviour only (no implemenation)
- said in other form, test only the end state of your code and that is consistent with the input state. Don't test the intermediate test / execution flows.
There was a problem hiding this comment.
On a quick overall scan, this test file doesn't seem to be using private API, so just suffix it with _test accordingly
|
|
||
| // Acquire returns an accepted hit or a lease with the old value (zero on a miss). | ||
| // Hits bypass active owners; otherwise callers wait for them. Defer Abort on returned leases. | ||
| // Nil accepts allows any cached value; predicates must be brief, read-only, and never reenter. |
There was a problem hiding this comment.
It seems you meant either only accepts or allows word.
Also, this description can be improved a bit since it is ab it abstract. For example:
// Hits bypass active owners; otherwise callers wait for them. Defer Abort on returned leases.I think it is hard to understand what that sentence is about, I assume active owners are leases, so maybe it is best if refer to them by that name.
I think the whole Acquire descriptions needs a full rewrite, being a lot specific of what's talking about.
There was a problem hiding this comment.
Detailed Acquire in an organized manner.
|
|
||
| // Cache stores immutable values with one active owner per key. | ||
| type Cache[K comparable, V any] struct { | ||
| mu sync.Mutex |
There was a problem hiding this comment.
Now the mutex seems to lock everyone independently if they are readers or writers. I see that Lease only wants to read, so there is a maybe a benefit for it.
| const ( | ||
| cacheHit cacheLookupKind = iota | ||
| cacheWait | ||
| cacheLoad |
There was a problem hiding this comment.
cacheLoad is a more of a cacheMiss right?
There was a problem hiding this comment.
Renamed to cacheLease for clarity. cacheLoad can happen when nothing is cached or when accepts rejects. A cache miss can also lead to a cache wait instead.
| flight := make(chan struct{}) | ||
| c.flights[*key] = flight | ||
| return cacheLookup[K, V]{ | ||
| kind: cacheLoad, value: value, |
There was a problem hiding this comment.
one param per arg to make it easier to read.
Style guide is:
- Either everyone (function params, call arguments, struct field initialization, etc) on the same line
- Or everyone seperated per line
// allowed
struct T {a: a, b: b, c: c}
//allowed
struct T {
a: a,
b: b,
c: c,
}
//allowed
struct T {
a: a, b: b, c: c,
}
//unallowed
struct T {
a: a, b:b,
c: c,
}
There was a problem hiding this comment.
Artifact of golint
| // Publish stores read-only data and releases ownership. Released leases are ignored. | ||
| func (w *Lease[K, V]) Publish(value V) { | ||
| cache := w.cache | ||
| cache.mu.Lock() | ||
| defer cache.mu.Unlock() | ||
| if cache.flights[w.key] != w.flight { | ||
| return | ||
| } | ||
| cache.records.Add(w.key, value) | ||
| cache.finishLocked(&w.key, w.flight) | ||
| } | ||
|
|
||
| // Abort releases ownership without changing the value. Repeated calls are safe. | ||
| func (w *Lease[K, V]) Abort() { | ||
| cache := w.cache | ||
| cache.mu.Lock() | ||
| defer cache.mu.Unlock() | ||
| if cache.flights[w.key] != w.flight { | ||
| return | ||
| } | ||
| cache.finishLocked(&w.key, w.flight) | ||
| } |
There was a problem hiding this comment.
On the topic of mutexes, I am not sure if Lease interacting directly with Cache internal types is the best design, but honestly finding it hard to think of a better way to model this.
Just a comment, not required change needs to be associated with it
| // Cache stores immutable values with one active owner per key. | ||
| type Cache[K comparable, V any] struct { | ||
| mu sync.Mutex | ||
| records *lru.SimpleCache[K, V] | ||
| flights map[K]chan struct{} | ||
| } | ||
|
|
||
| // Lease is the exclusive right to publish for a key, independent of eviction. | ||
| type Lease[K comparable, V any] struct { | ||
| cache *Cache[K, V] | ||
| key K | ||
| flight chan struct{} | ||
| } | ||
|
|
There was a problem hiding this comment.
Since both Cache and Lease types are meant to be used in tandem, I think a proper description, needs to be written for both of them, explaining what they are and their intended use. The also lack on specificity.
Note that by proper description I don't mean 3 paragraphs, I mean something that is concise and explains both functionalities well to the developer so he can use them without needing to read the code. This applies to the comment above as well.
The most important thing is that the description is well done, it is irrelevant if it is just one word, three sentences or three paragraphs. The description will be read n times and written just once, so you're optimizing for readers – don't waste their time with unnecessary words, don't waste their time by forcing them to go read your code to understand how to use it (and the former is a better trade-off than the latter).
Sometimes adding examples on how to use the cache can greatly complement a well written description.
There was a problem hiding this comment.
Added more detail of how they work together and an example usage under acquire.
|
Claude finished @danielntmd's task in 16m 35s —— View job Review completeScope: Correctness/concurrency: Nits:
No blockers. The design (single in-flight owner per key, eviction independent of active leases, non-mutating cache reads) is sound and well exercised by the |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
d0024d7 to
f7cda0b
Compare
|
Claude finished @danielntmd's task in 9m 56s —— View job Review completeScope:
This revision already resolves every open item from the earlier review rounds:
Design: single in-flight owner per key, non-mutating cache reads, eviction independent of active leases — sound and well covered by tests for publication, replacement, eviction-during-ownership, cancellation, and retry-after-abort. Nit (already acknowledged, no action needed for this PR): a No blockers. Nothing new to add beyond what's already been addressed in this thread. |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
|
Claude finished @danielntmd's task in 15m 6s —— View job Review completeScope: Checks performed:
Findings: none new. This PR has already been through several review rounds (rodrodros + automated bots), and every substantive item was addressed in the current revision:
Design review ( One design tradeoff, already raised by the automated reviewer and acknowledged by the author: a No blockers, no nits to add beyond what's already been resolved in this thread. |
Standalone PR ReviewPR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review. PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
2 participants
User description
User description
This PR introduces the foundation for sharing immutable, read-only block traces across RPC versions.
Retained Behavior:
Cache Behavior: