Simplify the allocate and commit logic #33811
Conversation
- Only clear the writing head when the list is empty. - Commit just updates the reading tail (renamed the commit head).
| BeginRead = 1, | ||
| BeginReadTenative = 2, | ||
| EndRead = 4, | ||
| WritingActive = 8 |
There was a problem hiding this comment.
Before a non-null _writeHead meant that the there was a write in progress. This state keeps track of that fact now.
| public bool IsReadingActive => (_state & State.BeginRead) == State.BeginRead; | ||
|
|
||
| [Flags] | ||
| internal enum State |
There was a problem hiding this comment.
Does Pipe pack better if this is a byte size?
internal enum State : byte
| { | ||
| BeginRead = 1, | ||
| BeginReadTenative = 2, | ||
| EndRead = 4, |
There was a problem hiding this comment.
Why do you need EndRead state? The only check it is used for can be done via checking for BeginRead
There was a problem hiding this comment.
We have extremely weird behavior here.
- If you call TryRead and that returns false, then you call AdvanceTo it's supposed to work. In this case, nothing is set. This might be the one case that isn't possible to represent without a state. Just checking for the absence of BeginRead isn't enough.
There was a problem hiding this comment.
I just looked at this again, here's the state machine:
BeginRead()
EndRead()
OK
BeginReadTentative()
EndRead()
OK
EndRead()
OK
BeginReadTentative()
BeginRead()
OK
BeginRead()
BeginRead() // Throws already reading
BeginRead()
BeginReadTentative() // Throws already reading
EndRead()
EndRead() // Throws no reading operation
There was a problem hiding this comment.
Why is a single EndRead() without beginread ok?
src/System.IO.Pipelines/src/System/IO/Pipelines/PipeOperationState.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| _state &= ~State.EndRead; | ||
| _state |= State.BeginReadTenative; |
There was a problem hiding this comment.
You can just get rid of the BeginReadTenative [sic] state entirely. The only place it's ever referenced is in EndRead() when it's unset. Nothing ever actually checks whether the state is set or not.
You should leave the BeginReadTentative() method in order to throw if BeginRead is set end to unset the EndRead state, but the state itself can be removed.
There was a problem hiding this comment.
Turns out that isn't quite true. Look at the state diagram here https://github.com/dotnet/corefx/pull/33811/files#r238908068
There was a problem hiding this comment.
Yeah. You could get rid of the EndRead state or the ReadingTentative state, but not both.
|
🆙📅 |
| @@ -423,7 +383,7 @@ internal void AdvanceReader(in SequencePosition consumed, in SequencePosition ex | |||
| AdvanceReader((BufferSegment)consumed.GetObject(), consumed.GetInteger(), (BufferSegment)examined.GetObject(), examined.GetInteger()); | |||
| } | |||
|
|
|||
| internal void AdvanceReader(BufferSegment consumedSegment, int consumedIndex, BufferSegment examinedSegment, int examinedIndex) | |||
| private void AdvanceReader(BufferSegment consumedSegment, int consumedIndex, BufferSegment examinedSegment, int examinedIndex) | |||
There was a problem hiding this comment.
Nobody calls it outside of this class
- Only clear the writing head when the list is empty. - Commit just updates the reading tail (renamed the commit head).
- Only clear the writing head when the list is empty. - Commit just updates the reading tail (renamed the commit head). Commit migrated from dotnet/corefx@07d1bb7
PS: I'm not super happy about the
PipeOperationStatechanges but I didn't want to change all of the tests. Suggestions welcome.cc @benaadams