-
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
Use non-allocating Int64 key generation #7413
Conversation
|
||
using Nethermind.Int256; | ||
|
||
namespace Nethermind.Core.Extensions; | ||
|
||
public static class Int64Extensions | ||
{ | ||
public static byte[] ToBigEndianByteArrayWithoutLeadingZeros(this long value) | ||
public static ReadOnlySpan<byte> MutateToBigEndianSpanWithoutLeadingZeros(ref this long 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.
WHy does it mutate the original value? Is that safe?
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.
Its needs to return the Span over valid stackspace so it takes the long by ref and creates the span over it; so its valid
The wrinkle is it also needs to change its byte order from little endian to big endian; trashing the local variable on the other side.
However there are only two instances where that's important; one I moved this call down one so is last use; and other I take a copy to a new local so the rest of the method can keep using the original 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.
wouldn't it be better to always stackalloc 8 bytes?
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.
Can't return the stackalloc'd to the caller as it will have been unwound so no longer valid. Caller needs to allocate the space
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 could through in an out var and then mutate that instead so
ReadOnlySpan<byte> ToBigEndianSpanWithoutLeadingZeros(this long value, out long bigEndian)
Then in use it would be:
key.ToBigEndianSpanWithoutLeadingZeros(out _)
but that's pretty horrible too
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 this one a lot better - not to cut yourself. Looks good enough to me. Do it on <T>
for all the numbers! :)
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.
Isn't a T constraint for ReverseEndianness
; which does seem a missed opportunity for the runtime
Contributes to #7392
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing