-
Notifications
You must be signed in to change notification settings - Fork 464
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
Feat/native 4byte tracer #6864
Feat/native 4byte tracer #6864
Conversation
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLike4byteTracerTests.cs
Outdated
Show resolved
Hide resolved
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.
Overall very good, just few nitpicks and potential ideas to consider.
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLike4byteTracerTests.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLike4byteTracerTests.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm.Test/Tracing/GethLike4byteTracerTests.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/GethLikeNativeTracerFactory.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/GethLikeNativeTxTracer.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/Tracers/Native4ByteTracer.cs
Outdated
Show resolved
Hide resolved
|
||
private void Store4ByteIds(ReadOnlyMemory<byte> input, int size) | ||
{ | ||
string _4byteId = input.Span[..4].ToHexString() + '-' + size; |
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.
micro-optimization: you could use string.create instead of concatenating strings, resulting in less allocations.
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 attempted to follow the approach used to create strings in StateTestTxTracer, however I've been having some difficulty trying to implement it correctly. I may reach out for some assistance if I can't figure it out
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.
Not sure if worth it as it makes it less readable:
static int GetDigitsBase10(int n) => n == 0 ? 1 : (int)Math.Floor(Math.Log10(Math.Abs(n)) + 1);
const int length = 4;
string _4byteId = string.Create(length * 2 + 1 + GetDigitsBase10(size), (input, size), (span, state) =>
{
ref char charsRef = ref MemoryMarshal.GetReference(span);
ReadOnlySpan<byte> bytes = state.input.Span[..length];
Bytes.OutputBytesToCharHex(ref MemoryMarshal.GetReference(bytes), length, ref charsRef, false, 0);
span[length * 2] = '-';
size.TryFormat(span.Slice(length * 2 + 1), out _);
});
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.
Oh cool! Since the main goal of this PR is to optimize performance for the tracers then I think it makes sense to use this, even if it's less readable. I'll add a comment explaining that it's just concatenating strings
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/Tracers/Native4ByteTracer.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/Tracers/Native4ByteTracer.cs
Outdated
Show resolved
Hide resolved
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.
Just a few small nitpicks, looks good as long as the script isn't finding any diffs
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/GethLikeNativeTracerFactory.cs
Outdated
Show resolved
Hide resolved
src/Nethermind/Nethermind.Evm/Tracing/GethStyle/Custom/Native/GethLikeBlockNativeTracer.cs
Outdated
Show resolved
Hide resolved
// } | ||
public sealed class Native4ByteTracer : GethLikeNativeTxTracer | ||
{ | ||
public const string _4byteTracer = "4byteTracer"; |
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: public variable starting with _
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.
Yeah I wasn't sure what to do here since the variable name starts with a number. Should I just rename the constant to be FourByteTracer
instead?
Changes
SELECTOR-CALLDATASIZE
and the values are number of occurrences of this key.4byteTracer_legacy
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
I repurposed a script that @Marchhilloriginally wrote for automating testing the JS tracer implementations. The script listens for new blocks/transactions, sends a request to get the 4byteTracer results, and compares the native and legacy tracer results and checks for any differences.
We'll also want to do some performance testing to ensure that the performance is significantly better than the JS implementation and comparable to Geth/Reth.
Documentation
Requires documentation update
Requires explanation in Release Notes