core, eth: use tracing.Hooks by value #32185
Conversation
omerfirmak
requested review from
fjl,
s1na,
lightclient,
rjl493456442 and
MariusVanDerWijden
as code owners
omerfirmak
force-pushed
the
dynamic-gas-trace
branch
from
f8a6fa4 to
a0fb55f
Compare
core/tracing/hooks.go
Outdated
| @@ -217,6 +217,16 @@ type Hooks struct { | |||
| OnBlockHashRead BlockHashReadHook | |||
| } | |||
|
|
|||
| // HooksState returns if any of the state events are being hooked | |||
| func (h *Hooks) HooksState() bool { | |||
There was a problem hiding this comment.
Maybe HasStateHooks ? HooksState is correct, but sounds a bit weird (double meaning of hooks and Hooks)
There was a problem hiding this comment.
Yeap, that definitely is easier to understand.
| @@ -217,6 +217,16 @@ type Hooks struct { | |||
| OnBlockHashRead BlockHashReadHook | |||
| } | |||
|
|
|||
| // HooksState returns if any of the state events are being hooked | |||
| func (h *Hooks) HooksState() bool { | |||
| return h.OnBalanceChange != nil || | |||
There was a problem hiding this comment.
I'm wondering if there's a way to do this, s.th. we can be sure that we will not forget about it, if we add new hooks
There was a problem hiding this comment.
Yeah, I wanted to achieve something more foolproof as well. My first choice was to compare against some zero value like h != Hooks{}, but that doesn't work when the type has function pointers inside.
There was a problem hiding this comment.
added a caution comment
There was a problem hiding this comment.
How about we define a bitmaps in the Hook struct and avoid the construction by struct assignment directly?
Then we should have the ability to check if State hooks, Chain hooks, Vm hooks have been specified somehow.
There was a problem hiding this comment.
can be sure that we will not forget about it, if we add new hooks
how would bitmaps help with the issue above?
core/vm/evm.go
Outdated
| evm.captureEnd(evm.depth, startGas, leftOverGas, ret, err) | ||
| }(gas) | ||
| } | ||
| evm.captureBegin(evm.depth, CALL, caller, addr, input, gas, value.ToBig()) |
There was a problem hiding this comment.
It means the tracer will be checked for every Call/Callcode/StaticCall, ... opcodes.
which was a single pointer check.
Not sure about the overhead
There was a problem hiding this comment.
yeah, put them behind an if check again. f3bae51
the main motivation was to make Contract.UseGas and Contract.RefundGas inlinable.
omerfirmak
force-pushed
the
dynamic-gas-trace
branch
from
f3bae51 to
57fa10b
Compare
| @@ -126,24 +126,24 @@ func (c *Contract) Caller() common.Address { | |||
| } | |||
|
|
|||
| // UseGas attempts the use gas and subtracts it and returns true on success | |||
| func (c *Contract) UseGas(gas uint64, logger *tracing.Hooks, reason tracing.GasChangeReason) (ok bool) { | |||
| func (c *Contract) UseGas(gas uint64, onGasChange tracing.GasChangeHook, reason tracing.GasChangeReason) (ok bool) { | |||
There was a problem hiding this comment.
The PR's motivation says it's about making UseGas inlinable. The diff here doesn't seem to necessitate changing Hooks to a value.
There was a problem hiding this comment.
What made UseGas not inlinable is the additional logger != nil check and the pointer de-referencing (logger.OnGasChange). I could've moved these to every callsite, but that ends up creating a mess everywhere. So ended up getting rid of the culprits (nil-check and de-referencing) entirely.
omerfirmak
force-pushed
the
dynamic-gas-trace
branch
2 times, most recently
from
61b6449 to
57fa10b
Compare
the main motivation was to make Contract.UseGas and Contract.RefundGas inlinable. But it also has the nice side affect of getting rid of the verbose
if tracer != nil && tracer.OnSomeEvent != nilpattern.