If not waiting for data, allocate memory before checking anything #22225
Conversation
ghost
added
the
| { | ||
| // If not waiting for data allocate another buffer immediately after flushing, | ||
| // to try and aquire some before the reader takes the lock. | ||
| buffer = input.GetMemory(MinAllocBufferSize); |
There was a problem hiding this comment.
Given that the flushTask should always be completed in the benchmark we're looking at, why do you expect calling GetMemory() here instead of the top of the loop to be substantially different? Is it so expensive to retrieve the FlushResult and check few more bools that it's likely to change the timing of GetMemory() relative to another thread waking up?
There was a problem hiding this comment.
I mention in dotnet/runtime#36992 (comment) that its basically running a data race; in combination with PR dotnet/runtime#36992 it just needs to set the IsWritingActive flag near the start of GetMemory() before the Reader takes the lock to avoid the lock contention.
I'm not sure it will work and if not then PipeWriter would need a FlushAsync(bool moreData = false) overload; where it doesn't switch out of writing when moreData is true, akin to the linux send(2) flag MSG_MORE, the Windows RioSend RIO_MSG_DEFER flag; or WSASend MSG_PARTIAL flag; which would allow it to either keep using the existing buffer or request a replacement larger one without having the data race.
There was a problem hiding this comment.
FlushAsync(bool moreData = false) would be better as Memory.Slice is cheaper than GetMemory() 😉
| @@ -209,7 +216,18 @@ private async Task ProcessReceives() | |||
|
|
|||
| input.Advance(bytesReceived); | |||
|
|
|||
| var flushTask = input.FlushAsync(); | |||
| var flushTask = input.FlushAsync(isMoreData: !_waitForData); | |||
There was a problem hiding this comment.
@benaadams @davidfowl Any chance we're getting the isMoreData: API in .NET 5?
| { | ||
| if (buffer.Length - bytesReceived >= MinAllocBufferSize) | ||
| { | ||
| buffer = buffer.Slice(bytesReceived); |
There was a problem hiding this comment.
The skipping of the call to GetMemory() doesn't even look like it should be allowed by the PipeWriter. Is this critical for perf?
There was a problem hiding this comment.
Its what the api would allow
|
@benaadams I'm closing this dotnet/runtime#37472 has also been closed. Hopefully we can optimize this more in .NET 6. |
amcasey
added
area-networking
To reduce contention between reader and writer on the MemoryPool (as no-delay in the no wait scenario) seen in dotnet/runtime#36956 (comment)
Its a race to see if it can get it before the Flush is picked up; but the Flush invalidatees outstanding Memory, so we can't do it before the Flush :-/