-
Notifications
You must be signed in to change notification settings - Fork 20k
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
eth/tracers: use non-threaded traceBlock for native tracers #24283
Conversation
I'm still debugging a bit what happens on
And
|
Triage discussion: merge ##24286 asap, fix this so it uses 'non-threaded' for native tracers but falls back to parallel tracing for js-based tracers |
f9385e8
to
5c75246
Compare
Some measurements Native tracingMaster, native
PR, native
JS but scopedMaster, js-scoped
PR, js-scoped
JS legacyMaster, js-legacy
PR, js-legacy
|
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
5c75246
to
26799d9
Compare
Co-authored-by: Martin Holst Swende <martin@swende.se>
26799d9
to
e111830
Compare
After this JS times stay the same (no change in logic) and as @holiman showed native tracing of blocks becomes faster. At least for more computational ones. prestateTracer stays roughly same speed as master.
|
eth/tracers/tracers.go
Outdated
@@ -42,6 +42,12 @@ type Tracer interface { | |||
Stop(err error) | |||
} | |||
|
|||
// JSTracer is implemented by tracers evaluating JS code. |
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 use this trick to detect JS tracers. It's not very elegant. I thought about adding a Type() method which is more general, or Name(), or something. But not sure realistically where else this logic would be required.
eth/tracers/api.go
Outdated
if config != nil && config.Tracer != nil && *config.Tracer != "" { | ||
t, err := New(*config.Tracer, nil, nil) | ||
if err != nil { | ||
return nil, err | ||
} | ||
if _, ok := t.(JSTracer); ok { | ||
return api.traceBlockParallel(ctx, block, statedb, config) | ||
} | ||
} |
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.
This whole thing is a bit wonky, but I guess we can live with it. The alternative would be that they register with some special attribute, and then you can query IsJs(*config.Tracer)
or ParallelTracing(*config.Tracer)
.
I'm fine with it as is, though
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.
Now that I've seen that you also needed to add that wonky interface just to distinguish the type, I think a less hacky solution would be to change the registration phase, so they register with some attriubute.
Then this whole thing can become less clunky, and also does not have to run the constructor twice
if config != nil && config.Tracer != nil{
if err, ok := IsInterpreted(config.Tracer); err != nil {
return nil, err
}else if ok{
return api.traceBlockParallel(ctx, block, statedb, config)
}
}
eth/tracers/tracers.go
Outdated
// JSTracer is implemented by tracers evaluating JS code. | ||
type JSTracer interface { | ||
Tracer | ||
IsJS() bool |
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.
Funky interface.
You can drop the return value, though
Ok now tracers signal whether they evaluate JS during registration. It's much less [wonky, hacky, clunky, funky] 😄 At the same time I refactored the registration code a bit. |
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.
LGTM
@@ -606,24 +607,64 @@ func (api *API) traceBlock(ctx context.Context, block *types.Block, config *Trac | |||
return nil, err | |||
} | |||
defer 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.
nitpick, please add a blank line afterwards.
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.
ok done
This makes non-JS tracers execute all block txs on a single goroutine. In the previous implementation, we used to prepare every tx pre-state on one goroutine, and then run the transactions again with tracing enabled. Native tracers are usually faster, so it is faster overall to use their output as the pre-state for tracing the next transaction. Co-authored-by: Sina Mahmoodi <itz.s1na@gmail.com>
This PR fixes #23205..
On master, when we trace a block, there are two things happening in parallel,
task
s, which contain a statedb and tx context info. Once they obtain a task, they to tracing on top of the given state.statedb.Copy
and then collects it into atask
and sends to the waiting tracer-threads.For some reason, this is not working as expected, and corruptions occur. Example block is given in #23205,
My repro script:
This script was executed against a node which had the stateroot, and was not importing blocks simultaneously.
Example output on
master
:This PR removs the multi-threading (with native tracing, the overhead of tracing is much lower, so it might make sense regardless. )
The same repro script with this PR produces a sequence of identical files;
Regarding speed differences, this PR actually seems to be performing better 👀
Master:
PR: