-
Notifications
You must be signed in to change notification settings - Fork 0
feat: saexec package
#20
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
Conversation
…l be much easier for everyone
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
saexec/saexec.go
Outdated
|
|
||
| snaps := e.executeScratchSpace.snaps | ||
| snaps.Disable() | ||
| snaps.Release() |
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.
Prior to Release, geth Journals the snapshot so that on restart is can be easily loaded. I don't think we necessarily need this, as we could just Cap(root, 0) as we are never going to reorg the state.
Should we be doing one of these here?
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.
Looking into this more closely, Cap(root, 0) seems to error if the root is the disk layer of the snap tree... How odd
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.
I've gone with Cap() because it's simpler (doesn't require committing the trie root returned by Journal()) and presumably persists less due to flattening. The Cap([disk root], 0) error should really be nil because it's a no-op, so I ignore errors under those circumstances. I could also just not call it, but the code is cleaner without the nested if, and there are no side effects.
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.
(As commented below, I think I'd prefer the check prior rather than the check after)
| // still leaks at shutdown. This is acceptable as we only ever have one | ||
| // [Executor], which we expect to be running for the entire life of the | ||
| // process. | ||
| goleak.IgnoreTopFunction("github.com/ava-labs/libevm/core/state/snapshot.(*diskLayer).generate"), |
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.
So, I'm not sure if there is any better way to avoid this... But in production, we shouldn't see this as a leak because this only happens:
- If a snapshot regeneration is needed on startup.
- If the disk layer hasn't been written to since the snapshot has finished generation.
In all of our tests I don't think we are actually triggering any snapshot writes right now, since we don't process enough layers for the Cap(root, 128) call to actually start flushing anything to disk.
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.
There's a race in the upstream implementation (still there at tip, just moved to a different file). The goleak error includes:
[Goroutine 25 in state chan receive, with github.com/ava-labs/libevm/core/state/snapshot.(*diskLayer).generate on top of the stack:
github.com/ava-labs/libevm/core/state/snapshot.(*diskLayer).generate(0x140000d0360, 0x140000dd5c0)
/.../go/pkg/mod/github.com/ava-labs/[email protected]/core/state/snapshot/generate.go:722 +0x51c
The blocking channel receive:
dl.lock.Lock()
dl.genMarker = nil
close(dl.genPending)
dl.lock.Unlock()
// Someone will be looking for us, wait it out
abort = <-dl.genAbort // <------ L#722 referenced by goleak
abort <- nilBut snapshot.Tree.Disable() bails early (because layer.genMarker was marked as nil above) and never sends to layer.genAbort.
layer.lock.RLock()
generating := layer.genMarker != nil // <------ Is happening after the above snippet unlocks
layer.lock.RUnlock()
if !generating {
// Generator is already aborted or finished
break
}
// If the base layer is generating, abort it
if layer.genAbort != nil {
abort := make(chan *generatorStats)
layer.genAbort <- abort
<-abort
}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.
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]> Signed-off-by: Arran Schlosberg <[email protected]>
StephenButtolph
left a comment
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.
I'm not confident that the state is being managed 100% correctly (w.r.t. flushing and shutdown). But I feel like we can make than an issue to followup on after merging.
Added issue #29 |
Introduces the
saexec.Executor, which manages a FIFO queue of blocks for execution.Recommended review order:
saexec/saexec.godefines theExecutorand getters.saexec/execution.goimplements the execution loopsaexec/subscriptions.goimplements notification points foreth_subscribesaexec/saexec_test.gotesting of the aboveCloses #14