Optimise precompile and create allocations#12105
Conversation
|
Claude finished @flcl42's task in 5m 11s —— View job PR Review: Optimise precompile and create allocations
SummaryTwo independent, correct optimizations. No critical or high severity findings.
|
| Severity | Finding |
|---|---|
| Low | Test only verifies same-instance caching; a cross-instance assertion would strengthen coverage of the static invariant (inline suggestion added) |
| Low | PR description body still contains the template placeholder — worth filling in for traceability |
No blocking issues. The logic is sound and the optimizations are correct.
| private static FrozenDictionary<AddressAsKey, CodeInfo> Precompiles | ||
| { | ||
| get => new Dictionary<AddressAsKey, CodeInfo> | ||
| private static readonly FrozenDictionary<AddressAsKey, CodeInfo> Precompiles = |
There was a problem hiding this comment.
This is a genuine bug fix, not just an optimization. The old get-only property was calling ToFrozenDictionary() on a freshly-constructed Dictionary on every call to GetPrecompiles(), allocating ~20 CodeInfo wrappers and building a new frozen dictionary each time. The static readonly field initializer runs exactly once at class initialization — correct and thread-safe under CLR static-init guarantees.
| private ReadOnlyMemory<byte> _returnDataBuffer = Array.Empty<byte>(); | ||
| protected VmState<TGasPolicy> _currentState; | ||
| protected ReadOnlyMemory<byte>? _previousCallResult; | ||
| protected Address? _previousCallAddressResult; |
There was a problem hiding this comment.
The invariant is: exactly one of _previousCallAddressResult and _previousCallResult is non-null when a value needs to be pushed, never both. This is correctly maintained across all code paths:
PrepareCreateData(success): sets address non-null, result nullTryChargeAndDepositCodefailure: sets result=BytesZero, address=null ✓HandleRevert: sets result=FailureBytes, address=null ✓HandleFailure: sets result=FailureBytes, address=null ✓PrepareNextCallFrame: clears both to null ✓HandleRegularReturn: sets result=status, address=null ✓
The optimization is safe: Address.Bytes returns a ReadOnlySpan<byte> over an immutable private readonly ValueAddress, so no defensive copy is needed. The pushed 20-byte span is identical to what previousState.Env.ExecutingAccount.Bytes.ToArray() produced before.
| FrozenDictionary<AddressAsKey, CodeInfo> precompilesAgain = provider.GetPrecompiles(); | ||
|
|
||
| Assert.That(precompilesAgain, Is.SameAs(precompiles)); | ||
| } |
There was a problem hiding this comment.
Consider also asserting that two different EthereumPrecompileProvider instances return the same reference — that would demonstrate the static part of the fix (not just repeated calls on the same instance):
| } | |
| [Test] | |
| public void GetPrecompiles_ReturnsCachedDictionary() | |
| { | |
| EthereumPrecompileProvider provider = new(); | |
| FrozenDictionary<AddressAsKey, CodeInfo> precompiles = provider.GetPrecompiles(); | |
| FrozenDictionary<AddressAsKey, CodeInfo> precompilesAgain = provider.GetPrecompiles(); | |
| Assert.That(precompilesAgain, Is.SameAs(precompiles)); | |
| } | |
| [Test] | |
| public void GetPrecompiles_ReturnsSameReferenceAcrossInstances() | |
| { | |
| EthereumPrecompileProvider first = new(); | |
| EthereumPrecompileProvider second = new(); | |
| Assert.That(second.GetPrecompiles(), Is.SameAs(first.GetPrecompiles())); | |
| } |
EVM Opcode Benchmark DiffAggregated runs: base=3, pr=3 Regressions (1)
|
Fixes Closes Resolves #
Please choose one of the keywords above to refer to the issue this PR solves followed by the issue number (e.g. Fixes #000). If no issue number, remove the line. Also, remove everything marked optional that is not applicable. Remove this note after reading.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?