-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Remove slabs from the slab memory pool #30732
Conversation
- Use 4K arrays on the pinned heap - I didn't rename it
|
||
Memory = MemoryMarshal.CreateFromPinnedArray(slab.PinnedArray, _offset, _length); | ||
var pinnedArray = GC.AllocateUninitializedArray<byte>(length, pinned: true); |
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.
https://docs.microsoft.com/en-us/dotnet/api/system.gc.allocateuninitializedarray?view=net-5.0 as an FYI to others reviewing.
We didn't zero-allocate the array before, correct?
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.
Correct
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.
Memory = MemoryMarshal.CreateFromPinnedArray(slab.PinnedArray, _offset, _length); | ||
var pinnedArray = GC.AllocateUninitializedArray<byte>(length, pinned: true); | ||
|
||
Memory = MemoryMarshal.CreateFromPinnedArray(pinnedArray, 0, pinnedArray.Length); | ||
} | ||
|
||
/// <summary> | ||
/// Back-reference to the memory pool which this block was allocated from. It may only be returned to this pool. | ||
/// </summary> | ||
public SlabMemoryPool Pool { get; } |
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.
Remove? Only spot I see this used is when BLOCK_LEASE_TRACKING
is defined.
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 used in dispose
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.
D'oh
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.
looks like a good change to me!
CC @cshung who's doing some perf work on POH |
/// This object cannot be instantiated outside of the static Create method | ||
/// </summary> | ||
internal MemoryPoolBlock(SlabMemoryPool pool, MemoryPoolSlab slab, int offset, int length) | ||
internal MemoryPoolBlock(SlabMemoryPool pool, int length) |
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.
SlabMemoryPool
-> SlablessMemoryPool
😋
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.
For real though, name change?
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 think I'll rename it to PinnedBlockPool or something like that
Investigating #27394
Looks slightly faster (allocation should be faster in general). I ran the plaintext benchmark.
Before:
After
The allocation rate is also lower (for more requests) in this run:
Before
After