-
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
Hunting allocations 3 - introduce AccountStruct #6605
Conversation
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.
You ike ternary operators very much 😄
# Conflicts: # src/Nethermind/Nethermind.State/StorageTree.cs
Nethermind.State/StateTree.cs(47,32): error CS0029: Cannot implicitly convert type 'System.ReadOnlySpan' to 'System.Span' |
@@ -8,7 +8,7 @@ namespace Nethermind.Core | |||
{ | |||
public class Account |
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.
Ah still exist?
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 are places where we need to put it on heap, not sure if worth paying for boxing?
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.
But will try to move some usages to AccountStruct in next PR.
# Conflicts: # src/Nethermind/Nethermind.Serialization.Json/EthereumJsonSerializer.cs # src/Nethermind/Nethermind.Serialization.Rlp/AccountDecoder.cs # src/Nethermind/Nethermind.State/StateReader.cs # src/Nethermind/Nethermind.State/StateTree.cs
public AccountStruct ToStruct() => new(Nonce, Balance, StorageRoot, CodeHash); | ||
} | ||
|
||
public readonly struct AccountStruct |
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 is a 128 bytes struct (0.125 kB); want to make sure are passing by in
etc everywhere otherwise big copies (if low GC)
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.
How this comment ended? I see that in lot of places this is returned, no TryGet
or ref semantics there? I'm asking as I'm applying these changes to Paprika and it looks that we're potentially copying to much here?
@@ -162,7 +162,7 @@ public ResultWrapper<IEnumerable<Address>> eth_accounts() | |||
return Task.FromResult(GetStateFailureResult<UInt256?>(header)); | |||
} | |||
|
|||
Account account = _stateReader.GetAccount(header!.StateRoot!, address); | |||
AccountStruct? account = _stateReader.GetAccount(header!.StateRoot!, address); |
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.
Does it have to be nullable? Extra byte it adds is problematic on copy size (128 bytes -> 129 bytes)
better to init with empy code hash and empty storage has and pass that back if null?
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 been thinking about it, how to avoid copies to. Maybe the answer is something like:
bool TryGetAccount(root, address, ref AccountStruct account)
not sure if we could write directly to stack allocated by the calling method?
} | ||
else | ||
{ | ||
storageRoot = rlpStream.DecodeValueKeccak()!.Value; |
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.
if for some reason DecodeValueKeccak() returns null, the underlaying value will probably be a zero-value struct(?)
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?