-
Notifications
You must be signed in to change notification settings - Fork 156
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
Feature/consumable array buffer writer pooling #59
Feature/consumable array buffer writer pooling #59
Conversation
@YairHalberstadt Can you squash all commits and base this on master? It seems to be based on your previous branch. |
It is. |
@YairHalberstadt also, change the ConsumableArrayBufferWriter to be byte specific. |
Remove parts of tests that are no longer deterministic.
8b1a17b
to
799bafe
Compare
@@ -349,62 +343,45 @@ public void GetMemoryWithSizeHint_InitSizeCtor(int sizeHint) | |||
public void GetMemoryAndSpan() |
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 did this test change so much?
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.
Since we're using the ArrayPool there's no guarantee the buffer will be clean, so I removed this check (The for loops).
For the same reason there's no guarantee that the new buffer wont happen to contain the same bytes as the old buffer. In fact it's quite likely due to the way these tests are set up, and happened by chance in one of my runs.
Assumptions about the size and capacity of the Buffer are now much weaker, so I had to change/remove them.
I'd like to see Consume return the buffer to the pool if everything has been consumed (or at least return to the original capacity ~ 256) |
Just a thought and probably due to my own ignorance, but would it be wiser to use MemoryPool here instead? I realize that MemoryPool just effectively passes through to ArrayPool at this point in time, but there has been discussion on optimizing it specifically and it does seem semantically more appropriate. |
Nope. It allocates an IMemoryOwner every time you rent, we optimized this in pipelines in 3.0 to directly use the array pool directly by default. |
Ah yeah, that does seem worse, in that case. |
@@ -132,6 +128,12 @@ public void Consume(int count) | |||
{ | |||
_index = 0; | |||
_consumedCount = 0; | |||
if (Capacity >= DefaultInitialBufferSize * 2) |
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 we want to return the buffer on consumed regardless of the size no?
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 it's the Default size why return it? The moment anyone wrote anything we would ask for it back, and if they've finished with it they should dispose...
Initial implementation of pooling for ConsumableArrayBufferWriter.
Before:
After:
Upper bound on benefits of pooling without any synchronization or resizing:
One issue with this approach is that it requires users to call Complete once they are done parsing. I don't know if this is considered part of the contract.
If not the _backlog will not be returned to the ArrayPool. One consequence of this is we might pool it long enough for it to go on gen2 heap, but then drop it, which could lead to more GC pressure. See this comment by Stephen Toub.