-
Notifications
You must be signed in to change notification settings - Fork 459
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
Perf/faster block load for receipt #5708
Conversation
return memory.Value.FasterToArray(); | ||
} | ||
|
||
public static byte[] FasterToArray(this in Memory<byte> memory) |
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.
Naming could cause confusion, normally you expect a copy.
Minor: is the code layout readable here? Would prefer ?
operator.
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.
What do you have in mind? AsArray
, AsArrayOrCopy
?
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 like .AsArray()
src/Nethermind/Nethermind.Serialization.Rlp/ReceiptRecoveryBlock.cs
Outdated
Show resolved
Hide resolved
private Transaction? _txBuffer; | ||
|
||
public Transaction GetNextTransaction() | ||
{ | ||
if (_transactions != null) | ||
{ | ||
return _transactions[_currentTransactionIndex++]; | ||
} | ||
|
||
Rlp.ValueDecoderContext decoderContext = new(_transactionData, true); | ||
decoderContext.Position = _currentTransactionPosition; | ||
TxDecoder.Instance.Decode(ref decoderContext, ref _txBuffer, RlpBehaviors.AllowUnsigned | RlpBehaviors.DisableLazyHash); | ||
_currentTransactionPosition = decoderContext.Position; | ||
|
||
return _txBuffer; |
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.
TransactionStruct
? and then have a generic decoder that would work with ITransaction
objects? This way we could reuse the 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.
Perhaps a follow up PR. Code in decoder is significant too.
Data = (Memory<byte>?)data, | ||
Type = Type, | ||
AccessList = TryGetAccessList(), | ||
ChainId = chainId, | ||
MaxFeePerDataGas = MaxFeePerDataGas, | ||
}; | ||
|
||
if (data is null) | ||
{ | ||
tx.Data = null; // Yes this is needed... really. Try a debugger. | ||
} | ||
|
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.
Data = data is null ? data : (Memory<byte>?)data,
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.
Nope, does not work.
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.
Whoa, why?
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 sorry meant: Data = data is null ? null : (Memory<byte>?)data
|
Did a mistake during benchmark as I forgot to remove a spinwait in KeccakF. Rerun benchmark. They are both CPU limited, likely at JSON serialization.
|
ReceiptRecoveryBlock
class which stores the tx's byte section as raw serializedMemory<byte>
.GetNextTransaction
which is used by block recovery.Transaction
instead of a ref struct, but reuse the object which turns out to be significant.DisableLazyHash
rlp param, so that transaction hash is calculated ahead of time as it will be needed.Transaction.Data
frombyte[]
toMemory<byte>
which can be backed by rocksdb's memory.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
eth_getLogs matches master.