fix: 1 mili sec delay for read and write#128418
Open
Neo-vortex wants to merge 1 commit into
Open
Conversation
3 tasks
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-io-ports |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #45635
Problem
When calling
ReadByte()multiple times while data is already available in thedriver buffer, each call incurs a minimum 1ms delay. For example:
The same issue affects writes. When writing chunks of data in sequence,
each
Write()call enqueues a request and waits for the next poll cycleto flush it, paying the same 1ms cost per write even when the driver's
send buffer has room and could accept the data immediately:
This affects any scenario where
port.BytesToRead > 1and the caller readsin a loop using
ReadByte()or any read smaller than the available data.Reported on Linux and WSL2. Windows is not affected as it uses a different
code path.
Root cause
Two bugs working together in
SerialStream.Unix.cs:Bug 1 —
IOLooponly processes one request per poll cycle.After a successful
PollEventscall confirms data is ready,DoIORequestiscalled once, completes one request, and returns. The loop then goes back to the
top and calls
PollEvents(timeout: 1ms)again for the next request — eventhough data is still sitting in the buffer ready to go. Each byte pays the full
1ms poll cost independently. This applies to both reads and writes.
Bug 2 —
ProcessReadandProcessWritereturn0for both "queue empty"and "EWOULDBLOCK".
Even if we tried to drain multiple requests in one shot, there was no way for
DoIORequestto know whether0meant "nothing left to read/write right now"vs "the queue is empty". This made it impossible to implement a correct drain
loop without risking spinning forever.
Fix
Four coupled changes, all required together:
ProcessRead— onEWOULDBLOCK, explicitly return-1instead of fallingthrough to
return 0at the bottom. This gives the caller a distinct signalthat no data is available right now, separate from the queue-empty case.
ProcessWrite— same change on the write side. Return-1onEWOULDBLOCKinstead of falling through.
DoIORequest— removeDebug.Assert(ret >= 0)since-1is now a validand meaningful return value. Propagate
-1immediately to the caller whenreceived from the processor delegate.
IOLoop— replace the singleDoIORequestcall with a drain loop for bothreads and writes. After a single successful
PollEvents, keep processingrequests until either the queue is empty (
0) or the driver signals no moredata right now (
-1)._totalBytesReadis updated inside the loop so it onlyaccumulates real positive reads.