Avoid empty segments from PipeReader.ReadAsync#72536
Avoid empty segments from PipeReader.ReadAsync#72536BrennanConroy merged 3 commits intodotnet:mainfrom
Conversation
ghost
assigned
| } | ||
|
|
||
| public void ResetMemory() | ||
| public void ResetMemory(bool preserveIndex = false) |
There was a problem hiding this comment.
Nit:
Maybe add a ClearMemory() method and rename this to Reset() and have it call ClearMemory(). Because this is doing more than just resetting memory, it's resetting the RunningIndex and the _next pointer too. It might make sense for ClearMemory() to keep the ResetMemory() name.
There was a problem hiding this comment.
👍 I was thinking along those lines when writing it. Good to know we're thinking that same thing.
| return newSegment; | ||
| } | ||
|
|
||
| private void SetupSegment(BufferSegment segment, int sizeHint) |
There was a problem hiding this comment.
Nit:
| private void SetupSegment(BufferSegment segment, int sizeHint) | |
| private void RentMemory(BufferSegment segment, int sizeHint) |
| } | ||
|
|
||
| BufferSegment newSegment = AllocateSegment(sizeHint); | ||
| if (_writingHead.Length == 0) |
There was a problem hiding this comment.
I would use _writingHeadBytesBuffered instead (but the logic would need to be reworked a bit)
There was a problem hiding this comment.
I think I tried using that field initially, but it gets set to 0 in Advance with a non-zero value, so you need to check some other value anyways. What's wrong with _writingHead.Length?
There was a problem hiding this comment.
Nothing wrong with it, it just avoids the indirection but this is fine.
|
Both test failures have tracking issues and are unrelated to this change, merging. |
3 participants
Fixes #30786
Reuses a
BufferSegmentif it is unused and a larger memory buffer is requested.There was already a memory pool test for this type of scenario, so I haven't added a new one.