-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Optimizations for KeyBuilder
#3598
base: master
Are you sure you want to change the base?
Conversation
public KeyBuilder AddBigEndian(int key) | ||
{ | ||
var data = new byte[sizeof(int)]; | ||
Span<byte> data = stackalloc byte[sizeof(int)]; |
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.
Are you sure this makes any difference? Have you measured these changes in isolation?
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.
Are you sure this makes any difference? Have you measured these changes in isolation?
Yes. This change avoids allocating memory from the heap.
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.
In general compilers can optimize such allocations out (but I'm not aware of C# compiler specifics, maybe it never does it and I just don't know), but if you've checked that this particular change is really worthwhile (and this means testing it in isolation from other changes in the same PR) then OK.
BinaryPrimitives.WriteInt32LittleEndian(data, id); | ||
|
||
stream = new(sizeof(int) + sizeof(byte) + keySizeHint); |
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.
byte
shouldn't be necessary, in general MaxStorageKeySize
limits the whole key with prefix included. That said, KeyBuilder
is somewhat special in that only native contracts use it. And native contracts can do things normal contracts can't (like #3596). Still, current native contracts never go beyond this limit.
BinaryPrimitives.WriteInt32LittleEndian(data, id); | ||
|
||
stream = new(sizeof(int) + sizeof(byte) + keySizeHint); |
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.
stream = new(sizeof(int) + sizeof(byte) + keySizeHint); | |
stream = new(keySizeHint); |
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="KeyBuilder"/> class. | ||
/// </summary> | ||
/// <param name="id">The id of the contract.</param> | ||
/// <param name="prefix">The prefix of the key.</param> | ||
public KeyBuilder(int id, byte prefix) | ||
/// <param name="keySizeHint">The hint of the storage key size(not including the id and prefix).</param> |
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.
/// <param name="keySizeHint">The hint of the storage key size(not including the id and prefix).</param> | |
/// <param name="keySizeHint">The hint of the storage key size(including the id and prefix).</param> |
Description
Optimizing memory allocation for
KeyBuilder
and add UTs for not-convered methodsBenchmarks:
Before:
After:
Type of change
Checklist: