From 580a164cc528b3c76e9d640869b9a62b6b9eab49 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 28 Feb 2023 11:47:03 +0000 Subject: [PATCH 01/23] SequenceReader; elide bounds checks by using ref-oriented approach --- .../System/Buffers/SequenceReader.Search.cs | 55 +++-- .../src/System/Buffers/SequenceReader.cs | 192 +++++++++--------- 2 files changed, 125 insertions(+), 122 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 27aef7056647dc..2781833ced9bc1 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -32,7 +32,7 @@ public bool TryReadTo(out ReadOnlySpan span, T delimiter, bool advancePastDel private bool TryReadToSlow(out ReadOnlySpan span, T delimiter, bool advancePastDelimiter) { - if (!TryReadToInternal(out ReadOnlySequence sequence, delimiter, advancePastDelimiter, CurrentSpan.Length - CurrentSpanIndex)) + if (!TryReadToInternal(out ReadOnlySequence sequence, delimiter, advancePastDelimiter, CurrentSpanLength - _currentSpanIndex)) { span = default; return false; @@ -198,7 +198,7 @@ private bool TryReadToInternal(out ReadOnlySequence sequence, T delimiter, bo Advance(skip); ReadOnlySpan remaining = UnreadSpan; - while (_moreData) + while (!End) { int index = remaining.IndexOf(delimiter); if (index != -1) @@ -243,7 +243,7 @@ public bool TryReadTo(out ReadOnlySequence sequence, T delimiter, T delimiter ReadOnlySpan remaining = UnreadSpan; bool priorEscape = false; - while (_moreData) + while (!End) { int index = remaining.IndexOf(delimiter); if (index != -1) @@ -344,7 +344,7 @@ public bool TryReadToAny(out ReadOnlySpan span, scoped ReadOnlySpan delimi private bool TryReadToAnySlow(out ReadOnlySpan span, scoped ReadOnlySpan delimiters, bool advancePastDelimiter) { - if (!TryReadToAnyInternal(out ReadOnlySequence sequence, delimiters, advancePastDelimiter, CurrentSpan.Length - CurrentSpanIndex)) + if (!TryReadToAnyInternal(out ReadOnlySequence sequence, delimiters, advancePastDelimiter, CurrentSpanLength - _currentSpanIndex)) { span = default; return false; @@ -578,11 +578,11 @@ public long AdvancePast(T value) { // Advance past all matches in the current span int i; - for (i = CurrentSpanIndex; i < CurrentSpan.Length && CurrentSpan[i].Equals(value); i++) + for (i = _currentSpanIndex; i < CurrentSpanLength && Unsafe.Add(ref CurrentSpanStart, i).Equals(value); i++) { } - int advanced = i - CurrentSpanIndex; + int advanced = i - _currentSpanIndex; if (advanced == 0) { // Didn't advance at all in this span, exit. @@ -593,7 +593,7 @@ public long AdvancePast(T value) // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. - } while (CurrentSpanIndex == 0 && !End); + } while (_currentSpanIndex == 0 && !End); return Consumed - start; } @@ -610,11 +610,11 @@ public long AdvancePastAny(scoped ReadOnlySpan values) { // Advance past all matches in the current span int i; - for (i = CurrentSpanIndex; i < CurrentSpan.Length && values.IndexOf(CurrentSpan[i]) != -1; i++) + for (i = _currentSpanIndex; i < CurrentSpanLength && values.IndexOf(Unsafe.Add(ref CurrentSpanStart, i)) != -1; i++) { } - int advanced = i - CurrentSpanIndex; + int advanced = i - _currentSpanIndex; if (advanced == 0) { // Didn't advance at all in this span, exit. @@ -625,7 +625,7 @@ public long AdvancePastAny(scoped ReadOnlySpan values) // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. - } while (CurrentSpanIndex == 0 && !End); + } while (_currentSpanIndex == 0 && !End); return Consumed - start; } @@ -642,16 +642,16 @@ public long AdvancePastAny(T value0, T value1, T value2, T value3) { // Advance past all matches in the current span int i; - for (i = CurrentSpanIndex; i < CurrentSpan.Length; i++) + for (i = _currentSpanIndex; i < CurrentSpanLength; i++) { - T value = CurrentSpan[i]; + T value = Unsafe.Add(ref CurrentSpanStart, i); if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2) && !value.Equals(value3)) { break; } } - int advanced = i - CurrentSpanIndex; + int advanced = i - _currentSpanIndex; if (advanced == 0) { // Didn't advance at all in this span, exit. @@ -662,7 +662,7 @@ public long AdvancePastAny(T value0, T value1, T value2, T value3) // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. - } while (CurrentSpanIndex == 0 && !End); + } while (_currentSpanIndex == 0 && !End); return Consumed - start; } @@ -679,16 +679,16 @@ public long AdvancePastAny(T value0, T value1, T value2) { // Advance past all matches in the current span int i; - for (i = CurrentSpanIndex; i < CurrentSpan.Length; i++) + for (i = _currentSpanIndex; i < CurrentSpanLength; i++) { - T value = CurrentSpan[i]; + T value = Unsafe.Add(ref CurrentSpanStart, i); if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2)) { break; } } - int advanced = i - CurrentSpanIndex; + int advanced = i - _currentSpanIndex; if (advanced == 0) { // Didn't advance at all in this span, exit. @@ -699,7 +699,7 @@ public long AdvancePastAny(T value0, T value1, T value2) // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. - } while (CurrentSpanIndex == 0 && !End); + } while (_currentSpanIndex == 0 && !End); return Consumed - start; } @@ -716,16 +716,16 @@ public long AdvancePastAny(T value0, T value1) { // Advance past all matches in the current span int i; - for (i = CurrentSpanIndex; i < CurrentSpan.Length; i++) + for (i = _currentSpanIndex; i < CurrentSpanLength; i++) { - T value = CurrentSpan[i]; + T value = Unsafe.Add(ref CurrentSpanStart, i); if (!value.Equals(value0) && !value.Equals(value1)) { break; } } - int advanced = i - CurrentSpanIndex; + int advanced = i - _currentSpanIndex; if (advanced == 0) { // Didn't advance at all in this span, exit. @@ -736,7 +736,7 @@ public long AdvancePastAny(T value0, T value1) // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. - } while (CurrentSpanIndex == 0 && !End); + } while (_currentSpanIndex == 0 && !End); return Consumed - start; } @@ -746,14 +746,13 @@ public long AdvancePastAny(T value0, T value1) /// public void AdvanceToEnd() { - if (_moreData) + if (!End) { - Consumed = Length; - CurrentSpan = default; - CurrentSpanIndex = 0; + _consumedAtStartOfCurrentSpan = Length; + WipeCurrentSpan(); + _currentPosition = Sequence.End; _nextPosition = default; - _moreData = false; } } @@ -768,7 +767,7 @@ public bool IsNext(T next, bool advancePast = false) if (End) return false; - if (CurrentSpan[CurrentSpanIndex].Equals(next)) + if (Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex).Equals(next)) { if (advancePast) { diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index b574a7298c6f0e..e01a53aae8363c 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -3,6 +3,7 @@ using System.Diagnostics; using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; namespace System.Buffers { @@ -10,8 +11,8 @@ namespace System.Buffers { private SequencePosition _currentPosition; private SequencePosition _nextPosition; - private bool _moreData; private readonly long _length; + private long _consumedAtStartOfCurrentSpan; /// /// Create a over the given . @@ -19,19 +20,16 @@ namespace System.Buffers [MethodImpl(MethodImplOptions.AggressiveInlining)] public SequenceReader(ReadOnlySequence sequence) { - CurrentSpanIndex = 0; - Consumed = 0; + _currentSpanIndex = 0; + _consumedAtStartOfCurrentSpan = 0; Sequence = sequence; _currentPosition = sequence.Start; _length = -1; - sequence.GetFirstSpan(out ReadOnlySpan first, out _nextPosition); - CurrentSpan = first; - _moreData = first.Length > 0; + SetCurrrentSpan(first); - if (!_moreData && !sequence.IsSingleSegment) + if (_currentSpanIndex == CurrentSpanLength && !sequence.IsSingleSegment) { - _moreData = true; GetNextSpan(); } } @@ -39,7 +37,7 @@ public SequenceReader(ReadOnlySequence sequence) /// /// True when there is no more data in the . /// - public readonly bool End => !_moreData; + public readonly bool End => _currentSpanIndex == CurrentSpanLength; // because of eager fetch, only ever true at EOF /// /// The underlying for the reader. @@ -58,17 +56,55 @@ public SequenceReader(ReadOnlySequence sequence) /// The current position in the . /// public readonly SequencePosition Position - => Sequence.GetPosition(CurrentSpanIndex, _currentPosition); + => Sequence.GetPosition(_currentSpanIndex, _currentPosition); /// /// The current segment in the as a span. /// - public ReadOnlySpan CurrentSpan { get; private set; } + public readonly ReadOnlySpan CurrentSpan + { + get + { +#if NET7_0_OR_GREATER + return MemoryMarshal.CreateReadOnlySpan(ref CurrentSpanStart, CurrentSpanLength); +#else + return _currentSpan; +#endif + } + } + + // only NET7+ can use 'ref T' field; use directly when possible; + // on down-level TFMs, use JIT-friendly property +#if NET7_0_OR_GREATER + private ref T CurrentSpanStart; + private int CurrentSpanLength, _currentSpanIndex; + private void SetCurrrentSpan(ReadOnlySpan span) + { + _consumedAtStartOfCurrentSpan += CurrentSpanLength; // account for previous + CurrentSpanStart = ref Unsafe.AsRef(in span.GetPinnableReference()); + CurrentSpanLength = span.Length; + _currentSpanIndex = 0; + } + private void WipeCurrentSpan() => CurrentSpanLength = _currentSpanIndex = 0; // no need to wipe ref +#else + private ReadOnlySpan _currentSpan; + private ref T CurrentSpanStart => ref Unsafe.AsRef(in current.GetPinnableReference()); + private void SetCurrentSpan(ReadOnlySpan span) + { + _currentSpan = span; + _currentSpanIndex = 0; + } + private void WipeCurrentSpan() + { + _currentSpan = default; + _currentSpanIndex = 0; + } +#endif /// /// The index in the . /// - public int CurrentSpanIndex { get; private set; } + public readonly int CurrentSpanIndex => _currentSpanIndex; /// /// The unread portion of the . @@ -76,13 +112,13 @@ public readonly SequencePosition Position public readonly ReadOnlySpan UnreadSpan { [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => CurrentSpan.Slice(CurrentSpanIndex); + get => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex), CurrentSpanLength - _currentSpanIndex); } /// /// The total number of 's processed by the reader. /// - public long Consumed { get; private set; } + public readonly long Consumed => _consumedAtStartOfCurrentSpan + _currentSpanIndex; /// /// Remaining 's in the reader's . @@ -113,16 +149,13 @@ public readonly long Length [MethodImpl(MethodImplOptions.AggressiveInlining)] public readonly bool TryPeek(out T value) { - if (_moreData) - { - value = CurrentSpan[CurrentSpanIndex]; - return true; - } - else + if (_currentSpanIndex == CurrentSpanLength) // only true at EOF due to eager read { value = default; return false; } + value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex); + return true; } /// @@ -137,7 +170,7 @@ public readonly bool TryPeek(long offset, out T value) ThrowHelper.ThrowArgumentOutOfRangeException_OffsetOutOfRange(); // If we've got data and offset is not out of bounds - if (!_moreData || Remaining <= offset) + if (_currentSpanIndex == CurrentSpanLength || Remaining <= offset) { value = default; return false; @@ -145,19 +178,19 @@ public readonly bool TryPeek(long offset, out T value) // Sum CurrentSpanIndex + offset could overflow as is but the value of offset should be very large // because we check Remaining <= offset above so to overflow we should have a ReadOnlySequence close to 8 exabytes - Debug.Assert(CurrentSpanIndex + offset >= 0); + Debug.Assert(_currentSpanIndex + offset >= 0); // If offset doesn't fall inside current segment move to next until we find correct one - if ((CurrentSpanIndex + offset) <= CurrentSpan.Length - 1) + if ((_currentSpanIndex + offset) <= CurrentSpanLength - 1) { Debug.Assert(offset <= int.MaxValue); - value = CurrentSpan[CurrentSpanIndex + (int)offset]; + value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex + (int)offset); return true; } else { - long remainingOffset = offset - (CurrentSpan.Length - CurrentSpanIndex); + long remainingOffset = offset - (CurrentSpanLength - _currentSpanIndex); SequencePosition nextPosition = _nextPosition; ReadOnlyMemory currentMemory; @@ -191,22 +224,20 @@ public readonly bool TryPeek(long offset, out T value) [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool TryRead(out T value) { - if (End) + switch (CurrentSpanLength - _currentSpanIndex) { - value = default; - return false; - } - - value = CurrentSpan[CurrentSpanIndex]; - CurrentSpanIndex++; - Consumed++; + case 0: // end of current span; since we move ahead eagerly: EOF + value = default; + return false; + case 1: // one left in current span + value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex); + GetNextSpan(); // move ahead eagerly + return true; + default: + value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex++); + return true; - if (CurrentSpanIndex >= CurrentSpan.Length) - { - GetNextSpan(); } - - return true; } /// @@ -230,15 +261,14 @@ public void Rewind(long count) Consumed -= count; - if (CurrentSpanIndex >= count) + if (_currentSpanIndex >= count) { - CurrentSpanIndex -= (int)count; - _moreData = true; + _currentSpanIndex -= (int)count; } else { // Current segment doesn't have enough data, scan backward through segments - RetreatToPreviousSpan(Consumed); + RetreatToPreviousSpan(Consumed - count); } } @@ -251,38 +281,37 @@ private void RetreatToPreviousSpan(long consumed) private void ResetReader() { - CurrentSpanIndex = 0; - Consumed = 0; + _consumedAtStartOfCurrentSpan = 0; _currentPosition = Sequence.Start; _nextPosition = _currentPosition; + // make sure SetCurrrentSpan doesn't count the existing + // span when advancing _runningIndex + WipeCurrentSpan(); + if (Sequence.TryGet(ref _nextPosition, out ReadOnlyMemory memory, advance: true)) { - _moreData = true; - if (memory.Length == 0) { - CurrentSpan = default; // No data in the first span, move to one with data GetNextSpan(); } else { - CurrentSpan = memory.Span; + SetCurrrentSpan(memory.Span); } } else { // No data in any spans and at end of sequence - _moreData = false; - CurrentSpan = default; + SetCurrrentSpan(default); } } /// /// Get the next segment with available data, if any. /// - private void GetNextSpan() + private bool GetNextSpan() { if (!Sequence.IsSingleSegment) { @@ -292,19 +321,14 @@ private void GetNextSpan() _currentPosition = previousNextPosition; if (memory.Length > 0) { - CurrentSpan = memory.Span; - CurrentSpanIndex = 0; - return; - } - else - { - CurrentSpan = default; - CurrentSpanIndex = 0; - previousNextPosition = _nextPosition; + SetCurrrentSpan(memory.Span); + return true; } + previousNextPosition = _nextPosition; } } - _moreData = false; + SetCurrrentSpan(default); + return false; } /// @@ -314,10 +338,9 @@ private void GetNextSpan() public void Advance(long count) { const long TooBigOrNegative = unchecked((long)0xFFFFFFFF80000000); - if ((count & TooBigOrNegative) == 0 && CurrentSpan.Length - CurrentSpanIndex > (int)count) + if ((count & TooBigOrNegative) == 0 && CurrentSpanLength - _currentSpanIndex > (int)count) { - CurrentSpanIndex += (int)count; - Consumed += count; + _currentSpanIndex += (int)count; } else { @@ -332,29 +355,14 @@ public void Advance(long count) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void AdvanceCurrentSpan(long count) { - Debug.Assert(count >= 0); + // worst case here is we end at the exact end of the current span + Debug.Assert(count >= 0 && _currentSpanIndex + count <= CurrentSpanLength); - Consumed += count; - CurrentSpanIndex += (int)count; - if (CurrentSpanIndex >= CurrentSpan.Length) + _currentSpanIndex += (int)count; + if (_currentSpanIndex == CurrentSpanLength) GetNextSpan(); } - /// - /// Only call this helper if you know that you are advancing in the current span - /// with valid count and there is no need to fetch the next one. - /// - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void AdvanceWithinSpan(long count) - { - Debug.Assert(count >= 0); - - Consumed += count; - CurrentSpanIndex += (int)count; - - Debug.Assert(CurrentSpanIndex < CurrentSpan.Length); - } - private void AdvanceToNextSpan(long count) { if (count < 0) @@ -362,27 +370,24 @@ private void AdvanceToNextSpan(long count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count); } - Consumed += count; - while (_moreData) + while (true) { - int remaining = CurrentSpan.Length - CurrentSpanIndex; + int remaining = CurrentSpanLength - _currentSpanIndex; if (remaining > count) { - CurrentSpanIndex += (int)count; + _currentSpanIndex += (int)count; count = 0; break; } - // As there may not be any further segments we need to - // push the current index to the end of the span. - CurrentSpanIndex += remaining; count -= remaining; Debug.Assert(count >= 0); - GetNextSpan(); - - if (count == 0) + // always want to move to next span here, even + // if we've consumed everything we want (to enforce + // eager fetch) + if (!GetNextSpan() || count == 0) { break; } @@ -391,7 +396,6 @@ private void AdvanceToNextSpan(long count) if (count != 0) { // Not enough data left- adjust for where we actually ended and throw - Consumed -= count; ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count); } } From dfe7241a211c766f725f9ddb4e0ac37cdf2cba25 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 28 Feb 2023 13:21:29 +0000 Subject: [PATCH 02/23] net6-: don't forget to increment _consumedAtStartOfCurrentSpan --- src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index e01a53aae8363c..f9ecac2f84e2b0 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -91,6 +91,7 @@ private void SetCurrrentSpan(ReadOnlySpan span) private ref T CurrentSpanStart => ref Unsafe.AsRef(in current.GetPinnableReference()); private void SetCurrentSpan(ReadOnlySpan span) { + _consumedAtStartOfCurrentSpan += CurrentSpanLength; // account for previous _currentSpan = span; _currentSpanIndex = 0; } From 481b49c51aa5c1c41169cc8e2e069f105015bc94 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 10:28:49 +0000 Subject: [PATCH 03/23] - don't use the Unsafe or MemoryMarshal evilness - unroll SequencePosition for packing - don't store next position (rarely used, easy to compute from current) - avoid bounds checking in span search - unify .ctor and ResetReader() --- .../System/Buffers/SequenceReader.Search.cs | 126 ++++++----- .../src/System/Buffers/SequenceReader.cs | 206 ++++++++---------- 2 files changed, 158 insertions(+), 174 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 2781833ced9bc1..142de5161144ac 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -32,7 +32,7 @@ public bool TryReadTo(out ReadOnlySpan span, T delimiter, bool advancePastDel private bool TryReadToSlow(out ReadOnlySpan span, T delimiter, bool advancePastDelimiter) { - if (!TryReadToInternal(out ReadOnlySequence sequence, delimiter, advancePastDelimiter, CurrentSpanLength - _currentSpanIndex)) + if (!TryReadToInternal(out ReadOnlySequence sequence, delimiter, advancePastDelimiter, _currentSpan.Length - _currentSpanIndex)) { span = default; return false; @@ -132,7 +132,7 @@ private bool TryReadToSlow(out ReadOnlySequence sequence, T delimiter, T deli // Found the delimiter. Move to it, slice, then move past it. AdvanceCurrentSpan(index); - sequence = Sequence.Slice(copy.Position, Position); + sequence = _sequence.Slice(copy.Position, Position); if (advancePastDelimiter) { Advance(1); @@ -209,7 +209,7 @@ private bool TryReadToInternal(out ReadOnlySequence sequence, T delimiter, bo AdvanceCurrentSpan(index); } - sequence = Sequence.Slice(copy.Position, Position); + sequence = _sequence.Slice(copy.Position, Position); if (advancePastDelimiter) { Advance(1); @@ -286,7 +286,7 @@ public bool TryReadTo(out ReadOnlySequence sequence, T delimiter, T delimiter Advance(index); } - sequence = Sequence.Slice(copy.Position, Position); + sequence = _sequence.Slice(copy.Position, Position); if (advancePastDelimiter) { Advance(1); @@ -344,7 +344,7 @@ public bool TryReadToAny(out ReadOnlySpan span, scoped ReadOnlySpan delimi private bool TryReadToAnySlow(out ReadOnlySpan span, scoped ReadOnlySpan delimiters, bool advancePastDelimiter) { - if (!TryReadToAnyInternal(out ReadOnlySequence sequence, delimiters, advancePastDelimiter, CurrentSpanLength - _currentSpanIndex)) + if (!TryReadToAnyInternal(out ReadOnlySequence sequence, delimiters, advancePastDelimiter, _currentSpan.Length - _currentSpanIndex)) { span = default; return false; @@ -387,7 +387,7 @@ private bool TryReadToAnyInternal(out ReadOnlySequence sequence, scoped ReadO AdvanceCurrentSpan(index); } - sequence = Sequence.Slice(copy.Position, Position); + sequence = _sequence.Slice(copy.Position, Position); if (advancePastDelimiter) { Advance(1); @@ -481,7 +481,7 @@ public bool TryReadTo(out ReadOnlySequence sequence, scoped ReadOnlySpan d // Probably a faster way to do this, potentially by avoiding the Advance in the previous TryReadTo call if (advanced) { - sequence = copy.Sequence.Slice(copy.Consumed, Consumed - copy.Consumed); + sequence = copy._sequence.Slice(copy.Consumed, Consumed - copy.Consumed); } if (advancePastDelimiter) @@ -520,7 +520,7 @@ public bool TryReadExact(int count, out ReadOnlySequence sequence) return false; } - sequence = Sequence.Slice(Position, count); + sequence = _sequence.Slice(Position, count); if (count != 0) { Advance(count); @@ -578,18 +578,18 @@ public long AdvancePast(T value) { // Advance past all matches in the current span int i; - for (i = _currentSpanIndex; i < CurrentSpanLength && Unsafe.Add(ref CurrentSpanStart, i).Equals(value); i++) + var unread = UnreadSpan; // eat the slice to avoid bounds checks + for (i = 0; i < unread.Length && unread[i].Equals(value); i++) { } - int advanced = i - _currentSpanIndex; - if (advanced == 0) + if (i == 0) { // Didn't advance at all in this span, exit. break; } - AdvanceCurrentSpan(advanced); + AdvanceCurrentSpan(i); // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. @@ -610,18 +610,18 @@ public long AdvancePastAny(scoped ReadOnlySpan values) { // Advance past all matches in the current span int i; - for (i = _currentSpanIndex; i < CurrentSpanLength && values.IndexOf(Unsafe.Add(ref CurrentSpanStart, i)) != -1; i++) + var unread = UnreadSpan; // eat the slice to avoid bounds checks + for (i = 0; i < unread.Length && values.IndexOf(unread[i]) != -1; i++) { } - int advanced = i - _currentSpanIndex; - if (advanced == 0) + if (i == 0) { // Didn't advance at all in this span, exit. break; } - AdvanceCurrentSpan(advanced); + AdvanceCurrentSpan(i); // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. @@ -642,23 +642,23 @@ public long AdvancePastAny(T value0, T value1, T value2, T value3) { // Advance past all matches in the current span int i; - for (i = _currentSpanIndex; i < CurrentSpanLength; i++) + var unread = UnreadSpan; // eat the slice to avoid bounds checks + for (i = 0; i < unread.Length; i++) { - T value = Unsafe.Add(ref CurrentSpanStart, i); + T value = unread[i]; if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2) && !value.Equals(value3)) { break; } } - int advanced = i - _currentSpanIndex; - if (advanced == 0) + if (i == 0) { // Didn't advance at all in this span, exit. break; } - AdvanceCurrentSpan(advanced); + AdvanceCurrentSpan(i); // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. @@ -679,23 +679,23 @@ public long AdvancePastAny(T value0, T value1, T value2) { // Advance past all matches in the current span int i; - for (i = _currentSpanIndex; i < CurrentSpanLength; i++) + var unread = UnreadSpan; // eat the slice to avoid bounds checks + for (i = 0; i < unread.Length; i++) { - T value = Unsafe.Add(ref CurrentSpanStart, i); + T value = unread[i]; if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2)) { break; } } - int advanced = i - _currentSpanIndex; - if (advanced == 0) + if (i == 0) { // Didn't advance at all in this span, exit. break; } - AdvanceCurrentSpan(advanced); + AdvanceCurrentSpan(i); // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. @@ -716,23 +716,23 @@ public long AdvancePastAny(T value0, T value1) { // Advance past all matches in the current span int i; - for (i = _currentSpanIndex; i < CurrentSpanLength; i++) + var unread = UnreadSpan; // eat the slice to avoid bounds checks + for (i = 0; i < unread.Length; i++) { - T value = Unsafe.Add(ref CurrentSpanStart, i); + T value = unread[i]; if (!value.Equals(value0) && !value.Equals(value1)) { break; } } - int advanced = i - _currentSpanIndex; - if (advanced == 0) + if (i == 0) { // Didn't advance at all in this span, exit. break; } - AdvanceCurrentSpan(advanced); + AdvanceCurrentSpan(i); // If we're at position 0 after advancing and not at the End, // we're in a new span and should continue the loop. @@ -749,10 +749,12 @@ public void AdvanceToEnd() if (!End) { _consumedAtStartOfCurrentSpan = Length; - WipeCurrentSpan(); + _currentSpanIndex = 0; + _currentSpan = default; - _currentPosition = Sequence.End; - _nextPosition = default; + var position = _sequence.End; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); } } @@ -767,7 +769,7 @@ public bool IsNext(T next, bool advancePast = false) if (End) return false; - if (Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex).Equals(next)) + if (_currentSpan[_currentSpanIndex].Equals(next)) { if (advancePast) { @@ -808,38 +810,46 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) Debug.Assert(currentSpan.Length < next.Length); int fullLength = next.Length; - SequencePosition nextPosition = _nextPosition; - - while (next.StartsWith(currentSpan)) + SequencePosition position = new(_currentPositionObject, _currentPositionInteger); + if (_sequence.TryGetBuffer(position, out var nextSegment, out var nextPosition)) { - if (next.Length == currentSpan.Length) - { - // Fully matched - if (advancePast) - { - Advance(fullLength); - } - return true; - } + // no-op removed by compiler; we don't need the first segment, but we + // also don't want the compiler creating a hidden local for a discard + // (we use this same local *later*) + _ = nextSegment; // suppress IDE0059 - // Need to check the next segment - while (true) + position = nextPosition; + while (next.StartsWith(currentSpan)) { - if (!Sequence.TryGet(ref nextPosition, out ReadOnlyMemory nextSegment, advance: true)) + if (next.Length == currentSpan.Length) { - // Nothing left - return false; + // Fully matched + if (advancePast) + { + Advance(fullLength); + } + return true; } - if (nextSegment.Length > 0) + // Need to check the next segment + while (true) { - next = next.Slice(currentSpan.Length); - currentSpan = nextSegment.Span; - if (currentSpan.Length > next.Length) + if (!_sequence.TryGetBuffer(position, out nextSegment, out nextPosition)) { - currentSpan = currentSpan.Slice(0, next.Length); + // Nothing left + return false; + } + position = nextPosition; + if (nextSegment.Length > 0) + { + next = next.Slice(currentSpan.Length); + currentSpan = nextSegment.Span; + if (currentSpan.Length > next.Length) + { + currentSpan = currentSpan.Slice(0, next.Length); + } + break; } - break; } } } diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index f9ecac2f84e2b0..af455f5d681fcb 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -9,10 +9,16 @@ namespace System.Buffers { public ref partial struct SequenceReader where T : unmanaged, IEquatable { - private SequencePosition _currentPosition; - private SequencePosition _nextPosition; + // keep all fields explicit to track (and pack) space; currently 72 bytes on x64 + + // deconstruct position for packing purposes + private object? _currentPositionObject; + private int _currentPositionInteger, _currentSpanIndex; + private readonly long _length; private long _consumedAtStartOfCurrentSpan; + private readonly ReadOnlySequence _sequence; + private ReadOnlySpan _currentSpan; /// /// Create a over the given . @@ -20,29 +26,21 @@ namespace System.Buffers [MethodImpl(MethodImplOptions.AggressiveInlining)] public SequenceReader(ReadOnlySequence sequence) { - _currentSpanIndex = 0; - _consumedAtStartOfCurrentSpan = 0; - Sequence = sequence; - _currentPosition = sequence.Start; + _sequence = sequence; _length = -1; - sequence.GetFirstSpan(out ReadOnlySpan first, out _nextPosition); - SetCurrrentSpan(first); - if (_currentSpanIndex == CurrentSpanLength && !sequence.IsSingleSegment) - { - GetNextSpan(); - } + ResetReader(); } /// /// True when there is no more data in the . /// - public readonly bool End => _currentSpanIndex == CurrentSpanLength; // because of eager fetch, only ever true at EOF + public readonly bool End => _currentSpanIndex == _currentSpan.Length; // because of eager fetch, only ever true at EOF /// /// The underlying for the reader. /// - public ReadOnlySequence Sequence { get; } + public readonly ReadOnlySequence Sequence => _sequence; /// /// Gets the unread portion of the . @@ -50,57 +48,27 @@ public SequenceReader(ReadOnlySequence sequence) /// /// The unread portion of the . /// - public readonly ReadOnlySequence UnreadSequence => Sequence.Slice(Position); + public readonly ReadOnlySequence UnreadSequence => _sequence.Slice(Position); /// /// The current position in the . /// public readonly SequencePosition Position - => Sequence.GetPosition(_currentSpanIndex, _currentPosition); + => _sequence.GetPosition(_currentSpanIndex, new SequencePosition(_currentPositionObject, _currentPositionInteger)); + // TODO: by eager-read, we *fully expect* that Position is inside the current span object (or very end for EOF); as such, + // we should be able to simplify to: => new SequencePosition(_currentPositionObject, _currentPositionInteger + _currentSpanIndex) /// /// The current segment in the as a span. /// - public readonly ReadOnlySpan CurrentSpan - { - get - { -#if NET7_0_OR_GREATER - return MemoryMarshal.CreateReadOnlySpan(ref CurrentSpanStart, CurrentSpanLength); -#else - return _currentSpan; -#endif - } - } + public readonly ReadOnlySpan CurrentSpan => _currentSpan; - // only NET7+ can use 'ref T' field; use directly when possible; - // on down-level TFMs, use JIT-friendly property -#if NET7_0_OR_GREATER - private ref T CurrentSpanStart; - private int CurrentSpanLength, _currentSpanIndex; - private void SetCurrrentSpan(ReadOnlySpan span) - { - _consumedAtStartOfCurrentSpan += CurrentSpanLength; // account for previous - CurrentSpanStart = ref Unsafe.AsRef(in span.GetPinnableReference()); - CurrentSpanLength = span.Length; - _currentSpanIndex = 0; - } - private void WipeCurrentSpan() => CurrentSpanLength = _currentSpanIndex = 0; // no need to wipe ref -#else - private ReadOnlySpan _currentSpan; - private ref T CurrentSpanStart => ref Unsafe.AsRef(in current.GetPinnableReference()); private void SetCurrentSpan(ReadOnlySpan span) { - _consumedAtStartOfCurrentSpan += CurrentSpanLength; // account for previous + _consumedAtStartOfCurrentSpan += _currentSpan.Length; // account for previous _currentSpan = span; _currentSpanIndex = 0; } - private void WipeCurrentSpan() - { - _currentSpan = default; - _currentSpanIndex = 0; - } -#endif /// /// The index in the . @@ -113,7 +81,7 @@ private void WipeCurrentSpan() public readonly ReadOnlySpan UnreadSpan { [MethodImpl(MethodImplOptions.AggressiveInlining)] - get => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex), CurrentSpanLength - _currentSpanIndex); + get => _currentSpan.Slice(_currentSpanIndex); } /// @@ -136,7 +104,7 @@ public readonly long Length if (_length < 0) { // Cast-away readonly to initialize lazy field - Unsafe.AsRef(in _length) = Sequence.Length; + Unsafe.AsRef(in _length) = _sequence.Length; } return _length; } @@ -150,12 +118,12 @@ public readonly long Length [MethodImpl(MethodImplOptions.AggressiveInlining)] public readonly bool TryPeek(out T value) { - if (_currentSpanIndex == CurrentSpanLength) // only true at EOF due to eager read + if (_currentSpanIndex == _currentSpan.Length) // only true at EOF due to eager read { value = default; return false; } - value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex); + value = _currentSpan[_currentSpanIndex]; return true; } @@ -171,7 +139,7 @@ public readonly bool TryPeek(long offset, out T value) ThrowHelper.ThrowArgumentOutOfRangeException_OffsetOutOfRange(); // If we've got data and offset is not out of bounds - if (_currentSpanIndex == CurrentSpanLength || Remaining <= offset) + if (_currentSpanIndex == _currentSpan.Length || Remaining <= offset) { value = default; return false; @@ -182,36 +150,41 @@ public readonly bool TryPeek(long offset, out T value) Debug.Assert(_currentSpanIndex + offset >= 0); // If offset doesn't fall inside current segment move to next until we find correct one - if ((_currentSpanIndex + offset) <= CurrentSpanLength - 1) + if ((_currentSpanIndex + offset) <= _currentSpan.Length - 1) { Debug.Assert(offset <= int.MaxValue); - value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex + (int)offset); + value = _currentSpan[_currentSpanIndex + (int)offset]; return true; } else { - long remainingOffset = offset - (CurrentSpanLength - _currentSpanIndex); - SequencePosition nextPosition = _nextPosition; - ReadOnlyMemory currentMemory; + long remainingOffset = offset - (_currentSpan.Length - _currentSpanIndex); + SequencePosition position = new(_currentPositionObject, _currentPositionInteger); - while (Sequence.TryGet(ref nextPosition, out currentMemory, advance: true)) + // first we need to skip past the current span (we already discounted that data) + if (_sequence.TryGetBuffer(position, out var currentMemory, out var next)) { - // Skip empty segment - if (currentMemory.Length > 0) + // now seek the additional buffers until we land in the segment we want + position = next; + while (_sequence.TryGetBuffer(position, out currentMemory, out next)) { - if (remainingOffset >= currentMemory.Length) + position = next; + // Skip empty segment + if (currentMemory.Length > 0) { - // Subtract current non consumed data - remainingOffset -= currentMemory.Length; - } - else - { - break; + if (remainingOffset >= currentMemory.Length) + { + // Subtract current non consumed data + remainingOffset -= currentMemory.Length; + } + else + { + break; + } } } } - value = currentMemory.Span[(int)remainingOffset]; return true; } @@ -225,17 +198,17 @@ public readonly bool TryPeek(long offset, out T value) [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool TryRead(out T value) { - switch (CurrentSpanLength - _currentSpanIndex) + switch (_currentSpan.Length - _currentSpanIndex) { case 0: // end of current span; since we move ahead eagerly: EOF value = default; return false; case 1: // one left in current span - value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex); + value = _currentSpan[_currentSpanIndex]; GetNextSpan(); // move ahead eagerly return true; default: - value = Unsafe.Add(ref CurrentSpanStart, _currentSpanIndex++); + value = _currentSpan[_currentSpanIndex++]; return true; } @@ -282,30 +255,15 @@ private void RetreatToPreviousSpan(long consumed) private void ResetReader() { + var position = _sequence.Start; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); _consumedAtStartOfCurrentSpan = 0; - _currentPosition = Sequence.Start; - _nextPosition = _currentPosition; - - // make sure SetCurrrentSpan doesn't count the existing - // span when advancing _runningIndex - WipeCurrentSpan(); - - if (Sequence.TryGet(ref _nextPosition, out ReadOnlyMemory memory, advance: true)) - { - if (memory.Length == 0) - { - // No data in the first span, move to one with data - GetNextSpan(); - } - else - { - SetCurrrentSpan(memory.Span); - } - } - else + _currentSpan = _sequence.FirstSpan; + _currentSpanIndex = 0; + if (_currentSpanIndex == _currentSpan.Length && !_sequence.IsSingleSegment) { - // No data in any spans and at end of sequence - SetCurrrentSpan(default); + GetNextSpan(); } } @@ -314,21 +272,26 @@ private void ResetReader() /// private bool GetNextSpan() { - if (!Sequence.IsSingleSegment) + if (!_sequence.IsSingleSegment) { - SequencePosition previousNextPosition = _nextPosition; - while (Sequence.TryGet(ref _nextPosition, out ReadOnlyMemory memory, advance: true)) + _consumedAtStartOfCurrentSpan += _currentSpan.Length; // account for previous + + SequencePosition position = new(_currentPositionObject, _currentPositionInteger); + while (_sequence.TryGetBuffer(position, out ReadOnlyMemory memory, out var next)) { - _currentPosition = previousNextPosition; if (memory.Length > 0) { - SetCurrrentSpan(memory.Span); + _currentSpan = memory.Span; + _currentSpanIndex = 0; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); return true; } - previousNextPosition = _nextPosition; + position = next; } } - SetCurrrentSpan(default); + _currentSpan = default; + _currentSpanIndex = 0; return false; } @@ -339,7 +302,7 @@ private bool GetNextSpan() public void Advance(long count) { const long TooBigOrNegative = unchecked((long)0xFFFFFFFF80000000); - if ((count & TooBigOrNegative) == 0 && CurrentSpanLength - _currentSpanIndex > (int)count) + if ((count & TooBigOrNegative) == 0 && _currentSpan.Length - _currentSpanIndex > (int)count) { _currentSpanIndex += (int)count; } @@ -357,10 +320,10 @@ public void Advance(long count) internal void AdvanceCurrentSpan(long count) { // worst case here is we end at the exact end of the current span - Debug.Assert(count >= 0 && _currentSpanIndex + count <= CurrentSpanLength); + Debug.Assert(count >= 0 && _currentSpanIndex + count <= _currentSpan.Length); _currentSpanIndex += (int)count; - if (_currentSpanIndex == CurrentSpanLength) + if (_currentSpanIndex == _currentSpan.Length) GetNextSpan(); } @@ -373,7 +336,7 @@ private void AdvanceToNextSpan(long count) while (true) { - int remaining = CurrentSpanLength - _currentSpanIndex; + int remaining = _currentSpan.Length - _currentSpanIndex; if (remaining > count) { @@ -440,18 +403,29 @@ internal readonly bool TryCopyMultisegment(Span destination) firstSpan.CopyTo(destination); int copied = firstSpan.Length; - SequencePosition next = _nextPosition; - while (Sequence.TryGet(ref next, out ReadOnlyMemory nextSegment, true)) + SequencePosition position = new(_currentPositionObject, _currentPositionInteger); + // first we need to skip past the current span (we already discounted that data) + if (_sequence.TryGetBuffer(position, out var nextSegment, out var next)) { - if (nextSegment.Length > 0) + // no-op removed by compiler; we don't need the first segment, but we + // also don't want the compiler creating a hidden local for a discard + // (we use this same local *later*) + _ = nextSegment; // suppress IDE0059 + + position = next; + while (_sequence.TryGetBuffer(position, out nextSegment, out next)) { - ReadOnlySpan nextSpan = nextSegment.Span; - int toCopy = Math.Min(nextSpan.Length, destination.Length - copied); - nextSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); - copied += toCopy; - if (copied >= destination.Length) + position = next; + if (nextSegment.Length > 0) { - break; + ReadOnlySpan nextSpan = nextSegment.Span; + int toCopy = Math.Min(nextSpan.Length, destination.Length - copied); + nextSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); + copied += toCopy; + if (copied >= destination.Length) + { + break; + } } } } From a5bf93da6b16bed12447f7917e7901dd19c7cc36 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 10:38:34 +0000 Subject: [PATCH 04/23] in search, avoid copying out the T value --- .../src/System/Buffers/SequenceReader.Search.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 142de5161144ac..6171c9fe0674d4 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -645,7 +645,7 @@ public long AdvancePastAny(T value0, T value1, T value2, T value3) var unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { - T value = unread[i]; + ref readonly T value = ref unread[i]; if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2) && !value.Equals(value3)) { break; @@ -682,7 +682,7 @@ public long AdvancePastAny(T value0, T value1, T value2) var unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { - T value = unread[i]; + ref readonly T value = ref unread[i]; if (!value.Equals(value0) && !value.Equals(value1) && !value.Equals(value2)) { break; @@ -719,7 +719,7 @@ public long AdvancePastAny(T value0, T value1) var unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { - T value = unread[i]; + ref readonly T value = ref unread[i]; if (!value.Equals(value0) && !value.Equals(value1)) { break; From 836bc9c45b40f9aa6b005d2380b608ddf10afe48 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 10:44:17 +0000 Subject: [PATCH 05/23] drop unused method --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index af455f5d681fcb..16949ff4355433 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -63,13 +63,6 @@ public readonly SequencePosition Position /// public readonly ReadOnlySpan CurrentSpan => _currentSpan; - private void SetCurrentSpan(ReadOnlySpan span) - { - _consumedAtStartOfCurrentSpan += _currentSpan.Length; // account for previous - _currentSpan = span; - _currentSpanIndex = 0; - } - /// /// The index in the . /// From 4e8d89c8ad0c15539579c03e865e05930720b488 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 11:56:56 +0000 Subject: [PATCH 06/23] reinstate next position; needed to avoid double-fetch in GetNextSpan --- .../System/Buffers/SequenceReader.Search.cs | 59 +++++++------- .../src/System/Buffers/SequenceReader.cs | 77 ++++++++----------- 2 files changed, 61 insertions(+), 75 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 6171c9fe0674d4..40162f1d92c519 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -755,6 +755,9 @@ public void AdvanceToEnd() var position = _sequence.End; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); + // and mimic default(Position) for next + _nextPositionObject = default; + _nextPositionInteger = default; } } @@ -810,46 +813,38 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) Debug.Assert(currentSpan.Length < next.Length); int fullLength = next.Length; - SequencePosition position = new(_currentPositionObject, _currentPositionInteger); - if (_sequence.TryGetBuffer(position, out var nextSegment, out var nextPosition)) - { - // no-op removed by compiler; we don't need the first segment, but we - // also don't want the compiler creating a hidden local for a discard - // (we use this same local *later*) - _ = nextSegment; // suppress IDE0059 + SequencePosition position = new(_nextPositionObject, _nextPositionInteger); - position = nextPosition; - while (next.StartsWith(currentSpan)) + while (next.StartsWith(currentSpan)) + { + if (next.Length == currentSpan.Length) { - if (next.Length == currentSpan.Length) + // Fully matched + if (advancePast) { - // Fully matched - if (advancePast) - { - Advance(fullLength); - } - return true; + Advance(fullLength); } + return true; + } - // Need to check the next segment - while (true) + // Need to check the next segment + while (true) + { + if (!_sequence.TryGetBuffer(position, out var nextSegment, out var nextPosition)) { - if (!_sequence.TryGetBuffer(position, out nextSegment, out nextPosition)) - { - // Nothing left - return false; - } - position = nextPosition; - if (nextSegment.Length > 0) + // Nothing left + return false; + } + position = nextPosition; + if (nextSegment.Length > 0) + { + next = next.Slice(currentSpan.Length); + currentSpan = nextSegment.Span; + if (currentSpan.Length > next.Length) { - next = next.Slice(currentSpan.Length); - currentSpan = nextSegment.Span; - if (currentSpan.Length > next.Length) - { - currentSpan = currentSpan.Slice(0, next.Length); - } - break; + currentSpan = currentSpan.Slice(0, next.Length); } + break; } } } diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 16949ff4355433..f60e90289f9d0d 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -9,11 +9,11 @@ namespace System.Buffers { public ref partial struct SequenceReader where T : unmanaged, IEquatable { - // keep all fields explicit to track (and pack) space; currently 72 bytes on x64 + // keep all fields explicit to track (and pack) space // deconstruct position for packing purposes - private object? _currentPositionObject; - private int _currentPositionInteger, _currentSpanIndex; + private object? _currentPositionObject, _nextPositionObject; + private int _currentPositionInteger, _nextPositionInteger, _currentSpanIndex; private readonly long _length; private long _consumedAtStartOfCurrentSpan; @@ -153,28 +153,23 @@ public readonly bool TryPeek(long offset, out T value) else { long remainingOffset = offset - (_currentSpan.Length - _currentSpanIndex); - SequencePosition position = new(_currentPositionObject, _currentPositionInteger); + SequencePosition position = new(_nextPositionObject, _nextPositionInteger); - // first we need to skip past the current span (we already discounted that data) - if (_sequence.TryGetBuffer(position, out var currentMemory, out var next)) + ReadOnlyMemory currentMemory; + while (_sequence.TryGetBuffer(position, out currentMemory, out var next)) { - // now seek the additional buffers until we land in the segment we want position = next; - while (_sequence.TryGetBuffer(position, out currentMemory, out next)) + // Skip empty segment + if (currentMemory.Length > 0) { - position = next; - // Skip empty segment - if (currentMemory.Length > 0) + if (remainingOffset >= currentMemory.Length) { - if (remainingOffset >= currentMemory.Length) - { - // Subtract current non consumed data - remainingOffset -= currentMemory.Length; - } - else - { - break; - } + // Subtract current non consumed data + remainingOffset -= currentMemory.Length; + } + else + { + break; } } } @@ -252,9 +247,12 @@ private void ResetReader() _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); _consumedAtStartOfCurrentSpan = 0; - _currentSpan = _sequence.FirstSpan; + _sequence.TryGetBuffer(position, out var memory, out var next); + _nextPositionObject = next.GetObject(); + _nextPositionInteger = next.GetInteger(); + _currentSpan = memory.Span; _currentSpanIndex = 0; - if (_currentSpanIndex == _currentSpan.Length && !_sequence.IsSingleSegment) + if (_currentSpan.IsEmpty && !_sequence.IsSingleSegment) { GetNextSpan(); } @@ -269,8 +267,9 @@ private bool GetNextSpan() { _consumedAtStartOfCurrentSpan += _currentSpan.Length; // account for previous - SequencePosition position = new(_currentPositionObject, _currentPositionInteger); - while (_sequence.TryGetBuffer(position, out ReadOnlyMemory memory, out var next)) + SequencePosition position = new(_nextPositionObject, _nextPositionInteger); + + while (_sequence.TryGetBuffer(position, out var memory, out var next)) { if (memory.Length > 0) { @@ -278,6 +277,8 @@ private bool GetNextSpan() _currentSpanIndex = 0; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); + _nextPositionObject = next.GetObject(); + _nextPositionInteger = next.GetInteger(); return true; } position = next; @@ -396,29 +397,19 @@ internal readonly bool TryCopyMultisegment(Span destination) firstSpan.CopyTo(destination); int copied = firstSpan.Length; - SequencePosition position = new(_currentPositionObject, _currentPositionInteger); - // first we need to skip past the current span (we already discounted that data) - if (_sequence.TryGetBuffer(position, out var nextSegment, out var next)) + SequencePosition position = new(_nextPositionObject, _nextPositionInteger); + while (_sequence.TryGetBuffer(position, out var nextSegment, out var next)) { - // no-op removed by compiler; we don't need the first segment, but we - // also don't want the compiler creating a hidden local for a discard - // (we use this same local *later*) - _ = nextSegment; // suppress IDE0059 - position = next; - while (_sequence.TryGetBuffer(position, out nextSegment, out next)) + if (nextSegment.Length > 0) { - position = next; - if (nextSegment.Length > 0) + ReadOnlySpan nextSpan = nextSegment.Span; + int toCopy = Math.Min(nextSpan.Length, destination.Length - copied); + nextSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); + copied += toCopy; + if (copied >= destination.Length) { - ReadOnlySpan nextSpan = nextSegment.Span; - int toCopy = Math.Min(nextSpan.Length, destination.Length - copied); - nextSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); - copied += toCopy; - if (copied >= destination.Length) - { - break; - } + break; } } } From f359dd02cd85a5c44909b21c9d4391d15febd508 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:01:51 +0000 Subject: [PATCH 07/23] use better API to get first span in ResetReader --- .../src/System/Buffers/SequenceReader.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index f60e90289f9d0d..70e549395e879f 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -246,14 +246,13 @@ private void ResetReader() var position = _sequence.Start; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); - _consumedAtStartOfCurrentSpan = 0; - _sequence.TryGetBuffer(position, out var memory, out var next); - _nextPositionObject = next.GetObject(); - _nextPositionInteger = next.GetInteger(); - _currentSpan = memory.Span; + _sequence.GetFirstSpan(out _currentSpan, out position); + _nextPositionObject = position.GetObject(); + _nextPositionInteger = position.GetInteger(); _currentSpanIndex = 0; + _consumedAtStartOfCurrentSpan = 0; if (_currentSpan.IsEmpty && !_sequence.IsSingleSegment) - { + { // edge-case; multi-segment with zero-length as first GetNextSpan(); } } From 454315e7923a7a5ed42f277fa37d7332485fc358 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:04:56 +0000 Subject: [PATCH 08/23] simplify Position getter --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 70e549395e879f..715bb39f9577af 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -53,10 +53,7 @@ public SequenceReader(ReadOnlySequence sequence) /// /// The current position in the . /// - public readonly SequencePosition Position - => _sequence.GetPosition(_currentSpanIndex, new SequencePosition(_currentPositionObject, _currentPositionInteger)); - // TODO: by eager-read, we *fully expect* that Position is inside the current span object (or very end for EOF); as such, - // we should be able to simplify to: => new SequencePosition(_currentPositionObject, _currentPositionInteger + _currentSpanIndex) + public readonly SequencePosition Position => new(_currentPositionObject, _currentPositionInteger + _currentSpanIndex); // since index in same segment, this is valid /// /// The current segment in the as a span. From d001302d6b62a81657aff55034b69095d1c248dd Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:11:28 +0000 Subject: [PATCH 09/23] move reset logic to constructor; use constructor in ResetReader --- .../src/System/Buffers/SequenceReader.cs | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 715bb39f9577af..f5f7728b74cf7b 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -3,7 +3,6 @@ using System.Diagnostics; using System.Runtime.CompilerServices; -using System.Runtime.InteropServices; namespace System.Buffers { @@ -29,7 +28,18 @@ public SequenceReader(ReadOnlySequence sequence) _sequence = sequence; _length = -1; - ResetReader(); + var position = _sequence.Start; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); + _sequence.GetFirstSpan(out _currentSpan, out position); + _nextPositionObject = position.GetObject(); + _nextPositionInteger = position.GetInteger(); + _currentSpanIndex = 0; + _consumedAtStartOfCurrentSpan = 0; + if (_currentSpan.IsEmpty && !_sequence.IsSingleSegment) + { // edge-case; multi-segment with zero-length as first + GetNextSpan(); + } } /// @@ -240,18 +250,14 @@ private void RetreatToPreviousSpan(long consumed) private void ResetReader() { - var position = _sequence.Start; - _currentPositionObject = position.GetObject(); - _currentPositionInteger = position.GetInteger(); - _sequence.GetFirstSpan(out _currentSpan, out position); - _nextPositionObject = position.GetObject(); - _nextPositionInteger = position.GetInteger(); - _currentSpanIndex = 0; - _consumedAtStartOfCurrentSpan = 0; - if (_currentSpan.IsEmpty && !_sequence.IsSingleSegment) - { // edge-case; multi-segment with zero-length as first - GetNextSpan(); - } + // preserve the length - it can be relatively expensive to calculate on demand + var length = _length; + + // reset all state + this = new(_sequence); + + // reinstate the length + Unsafe.AsRef(_length) = length; } /// From 4ba4be631a78804c2444b150b7f7a643703ee3e6 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:14:56 +0000 Subject: [PATCH 10/23] always track consumed span when moving to next, even for single segment --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index f5f7728b74cf7b..0c498b58cab8f1 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -265,10 +265,9 @@ private void ResetReader() /// private bool GetNextSpan() { + _consumedAtStartOfCurrentSpan += _currentSpan.Length; // track consumed length if (!_sequence.IsSingleSegment) { - _consumedAtStartOfCurrentSpan += _currentSpan.Length; // account for previous - SequencePosition position = new(_nextPositionObject, _nextPositionInteger); while (_sequence.TryGetBuffer(position, out var memory, out var next)) From ec519f7d6ae1e893584f4a80ba063f22cb010e3f Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:27:21 +0000 Subject: [PATCH 11/23] for single-segment cases, length is trivial; record length at EOF; update position at EOF --- .../src/System/Buffers/SequenceReader.cs | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 0c498b58cab8f1..060bf345d1bd2f 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -26,19 +26,28 @@ namespace System.Buffers public SequenceReader(ReadOnlySequence sequence) { _sequence = sequence; - _length = -1; - var position = _sequence.Start; + var position = sequence.Start; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); - _sequence.GetFirstSpan(out _currentSpan, out position); + sequence.GetFirstSpan(out _currentSpan, out position); _nextPositionObject = position.GetObject(); _nextPositionInteger = position.GetInteger(); _currentSpanIndex = 0; _consumedAtStartOfCurrentSpan = 0; - if (_currentSpan.IsEmpty && !_sequence.IsSingleSegment) - { // edge-case; multi-segment with zero-length as first - GetNextSpan(); + + if (sequence.IsSingleSegment) + { + _length = _currentSpan.Length; + } + else + { + _length = -1; // computed on-demand + if (_currentSpan.IsEmpty) + { + // edge-case; multi-segment with zero-length as first + GetNextSpan(); + } } } @@ -266,9 +275,10 @@ private void ResetReader() private bool GetNextSpan() { _consumedAtStartOfCurrentSpan += _currentSpan.Length; // track consumed length + SequencePosition position; if (!_sequence.IsSingleSegment) { - SequencePosition position = new(_nextPositionObject, _nextPositionInteger); + position = new(_nextPositionObject, _nextPositionInteger); while (_sequence.TryGetBuffer(position, out var memory, out var next)) { @@ -285,8 +295,16 @@ private bool GetNextSpan() position = next; } } + + // at EOF + position = _sequence.End; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); + _nextPositionObject = default; + _nextPositionInteger = default; _currentSpan = default; _currentSpanIndex = 0; + Unsafe.AsRef(in _length) = _consumedAtStartOfCurrentSpan; // since we know it, avoid later cost if Length accessed return false; } From f6d17c4a8b16f0e59f22bbd1b0cf16228b149e37 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 12:28:30 +0000 Subject: [PATCH 12/23] use named parameter to clarify meaning --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 060bf345d1bd2f..14c674549e80f7 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -30,7 +30,7 @@ public SequenceReader(ReadOnlySequence sequence) var position = sequence.Start; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); - sequence.GetFirstSpan(out _currentSpan, out position); + sequence.GetFirstSpan(out _currentSpan, next: out position); _nextPositionObject = position.GetObject(); _nextPositionInteger = position.GetInteger(); _currentSpanIndex = 0; From 7a17388ff162158d68c924b7f6ceb191db2d8aa0 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Wed, 1 Mar 2023 14:30:51 +0000 Subject: [PATCH 13/23] explicit types (IDE0008) --- .../src/System/Buffers/SequenceReader.Search.cs | 14 +++++++------- .../src/System/Buffers/SequenceReader.cs | 10 +++++----- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 40162f1d92c519..37248594a8f640 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -578,7 +578,7 @@ public long AdvancePast(T value) { // Advance past all matches in the current span int i; - var unread = UnreadSpan; // eat the slice to avoid bounds checks + ReadOnlySpan unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length && unread[i].Equals(value); i++) { } @@ -610,7 +610,7 @@ public long AdvancePastAny(scoped ReadOnlySpan values) { // Advance past all matches in the current span int i; - var unread = UnreadSpan; // eat the slice to avoid bounds checks + ReadOnlySpan unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length && values.IndexOf(unread[i]) != -1; i++) { } @@ -642,7 +642,7 @@ public long AdvancePastAny(T value0, T value1, T value2, T value3) { // Advance past all matches in the current span int i; - var unread = UnreadSpan; // eat the slice to avoid bounds checks + ReadOnlySpan unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { ref readonly T value = ref unread[i]; @@ -679,7 +679,7 @@ public long AdvancePastAny(T value0, T value1, T value2) { // Advance past all matches in the current span int i; - var unread = UnreadSpan; // eat the slice to avoid bounds checks + ReadOnlySpan unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { ref readonly T value = ref unread[i]; @@ -716,7 +716,7 @@ public long AdvancePastAny(T value0, T value1) { // Advance past all matches in the current span int i; - var unread = UnreadSpan; // eat the slice to avoid bounds checks + ReadOnlySpan unread = UnreadSpan; // eat the slice to avoid bounds checks for (i = 0; i < unread.Length; i++) { ref readonly T value = ref unread[i]; @@ -752,7 +752,7 @@ public void AdvanceToEnd() _currentSpanIndex = 0; _currentSpan = default; - var position = _sequence.End; + SequencePosition position = _sequence.End; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); // and mimic default(Position) for next @@ -830,7 +830,7 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) // Need to check the next segment while (true) { - if (!_sequence.TryGetBuffer(position, out var nextSegment, out var nextPosition)) + if (!_sequence.TryGetBuffer(position, out ReadOnlyMemory nextSegment, out SequencePosition nextPosition)) { // Nothing left return false; diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 14c674549e80f7..de94977ed7b758 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -27,7 +27,7 @@ public SequenceReader(ReadOnlySequence sequence) { _sequence = sequence; - var position = sequence.Start; + SequencePosition position = sequence.Start; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); sequence.GetFirstSpan(out _currentSpan, next: out position); @@ -172,7 +172,7 @@ public readonly bool TryPeek(long offset, out T value) SequencePosition position = new(_nextPositionObject, _nextPositionInteger); ReadOnlyMemory currentMemory; - while (_sequence.TryGetBuffer(position, out currentMemory, out var next)) + while (_sequence.TryGetBuffer(position, out currentMemory, out SequencePosition next)) { position = next; // Skip empty segment @@ -260,7 +260,7 @@ private void RetreatToPreviousSpan(long consumed) private void ResetReader() { // preserve the length - it can be relatively expensive to calculate on demand - var length = _length; + long length = _length; // reset all state this = new(_sequence); @@ -280,7 +280,7 @@ private bool GetNextSpan() { position = new(_nextPositionObject, _nextPositionInteger); - while (_sequence.TryGetBuffer(position, out var memory, out var next)) + while (_sequence.TryGetBuffer(position, out ReadOnlyMemory memory, out SequencePosition next)) { if (memory.Length > 0) { @@ -417,7 +417,7 @@ internal readonly bool TryCopyMultisegment(Span destination) int copied = firstSpan.Length; SequencePosition position = new(_nextPositionObject, _nextPositionInteger); - while (_sequence.TryGetBuffer(position, out var nextSegment, out var next)) + while (_sequence.TryGetBuffer(position, out ReadOnlyMemory nextSegment, out SequencePosition next)) { position = next; if (nextSegment.Length > 0) From 478115522420a28043093ddec8571bbee73ae457 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 13:05:47 +0000 Subject: [PATCH 14/23] - fix increment bug at end of single-segment buffers - implement custom buffer advance API; avoids need to store "next", plus faster - add position validity assertions --- .../System/Buffers/SequenceReader.Search.cs | 31 ++-- .../src/System/Buffers/SequenceReader.cs | 160 ++++++++++-------- 2 files changed, 104 insertions(+), 87 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 37248594a8f640..9bdc2b257a8c6c 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -746,6 +746,7 @@ public long AdvancePastAny(T value0, T value1) /// public void AdvanceToEnd() { + AssertValidPosition(); if (!End) { _consumedAtStartOfCurrentSpan = Length; @@ -755,9 +756,8 @@ public void AdvanceToEnd() SequencePosition position = _sequence.End; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); - // and mimic default(Position) for next - _nextPositionObject = default; - _nextPositionInteger = default; + + AssertValidPosition(); } } @@ -813,7 +813,7 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) Debug.Assert(currentSpan.Length < next.Length); int fullLength = next.Length; - SequencePosition position = new(_nextPositionObject, _nextPositionInteger); + SequencePosition position = Position; while (next.StartsWith(currentSpan)) { @@ -828,24 +828,13 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) } // Need to check the next segment - while (true) + if (!TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) { - if (!_sequence.TryGetBuffer(position, out ReadOnlyMemory nextSegment, out SequencePosition nextPosition)) - { - // Nothing left - return false; - } - position = nextPosition; - if (nextSegment.Length > 0) - { - next = next.Slice(currentSpan.Length); - currentSpan = nextSegment.Span; - if (currentSpan.Length > next.Length) - { - currentSpan = currentSpan.Slice(0, next.Length); - } - break; - } + return false; // no more data + } + if (currentSpan.Length > next.Length) + { + currentSpan = currentSpan.Slice(0, next.Length); } } diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index de94977ed7b758..33092d6bc92d47 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -11,8 +11,8 @@ namespace System.Buffers // keep all fields explicit to track (and pack) space // deconstruct position for packing purposes - private object? _currentPositionObject, _nextPositionObject; - private int _currentPositionInteger, _nextPositionInteger, _currentSpanIndex; + private object? _currentPositionObject; + private int _currentPositionInteger, _currentSpanIndex; private readonly long _length; private long _consumedAtStartOfCurrentSpan; @@ -30,9 +30,7 @@ public SequenceReader(ReadOnlySequence sequence) SequencePosition position = sequence.Start; _currentPositionObject = position.GetObject(); _currentPositionInteger = position.GetInteger(); - sequence.GetFirstSpan(out _currentSpan, next: out position); - _nextPositionObject = position.GetObject(); - _nextPositionInteger = position.GetInteger(); + _currentSpan = sequence.FirstSpan; _currentSpanIndex = 0; _consumedAtStartOfCurrentSpan = 0; @@ -46,9 +44,11 @@ public SequenceReader(ReadOnlySequence sequence) if (_currentSpan.IsEmpty) { // edge-case; multi-segment with zero-length as first - GetNextSpan(); + TryGetNextSpan(); } } + + AssertValidPosition(); } /// @@ -127,6 +127,7 @@ public readonly long Length [MethodImpl(MethodImplOptions.AggressiveInlining)] public readonly bool TryPeek(out T value) { + AssertValidPosition(); if (_currentSpanIndex == _currentSpan.Length) // only true at EOF due to eager read { value = default; @@ -144,6 +145,7 @@ public readonly bool TryPeek(out T value) /// true if the reader is not at its end and the peek operation succeeded; false if at the end of the reader. public readonly bool TryPeek(long offset, out T value) { + AssertValidPosition(); if (offset < 0) ThrowHelper.ThrowArgumentOutOfRangeException_OffsetOutOfRange(); @@ -169,27 +171,22 @@ public readonly bool TryPeek(long offset, out T value) else { long remainingOffset = offset - (_currentSpan.Length - _currentSpanIndex); - SequencePosition position = new(_nextPositionObject, _nextPositionInteger); + SequencePosition position = Position; - ReadOnlyMemory currentMemory; - while (_sequence.TryGetBuffer(position, out currentMemory, out SequencePosition next)) + ReadOnlySpan currentSpan = default; + while (TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) { - position = next; - // Skip empty segment - if (currentMemory.Length > 0) + if (remainingOffset >= currentSpan.Length) { - if (remainingOffset >= currentMemory.Length) - { - // Subtract current non consumed data - remainingOffset -= currentMemory.Length; - } - else - { - break; - } + // Subtract current non consumed data + remainingOffset -= currentSpan.Length; + } + else + { + break; } } - value = currentMemory.Span[(int)remainingOffset]; + value = currentSpan[(int)remainingOffset]; return true; } } @@ -202,17 +199,20 @@ public readonly bool TryPeek(long offset, out T value) [MethodImpl(MethodImplOptions.AggressiveInlining)] public bool TryRead(out T value) { + AssertValidPosition(); switch (_currentSpan.Length - _currentSpanIndex) { case 0: // end of current span; since we move ahead eagerly: EOF value = default; return false; case 1: // one left in current span - value = _currentSpan[_currentSpanIndex]; - GetNextSpan(); // move ahead eagerly + value = _currentSpan[_currentSpanIndex++]; + TryGetNextSpan(); // move ahead eagerly + AssertValidPosition(); return true; default: value = _currentSpan[_currentSpanIndex++]; + AssertValidPosition(); return true; } @@ -242,6 +242,7 @@ public void Rewind(long count) if (_currentSpanIndex >= count) { _currentSpanIndex -= (int)count; + AssertValidPosition(); } else { @@ -272,39 +273,64 @@ private void ResetReader() /// /// Get the next segment with available data, if any. /// - private bool GetNextSpan() + private bool TryGetNextSpan() + { + int lastLength = _currentSpan.Length; + SequencePosition position = Position; + if (!TryGetNextBuffer(in _sequence, ref position, ref _currentSpan)) + { + return false; + } + _consumedAtStartOfCurrentSpan += lastLength; // track consumed length + _currentSpanIndex = 0; + _currentPositionObject = position.GetObject(); + _currentPositionInteger = position.GetInteger(); + + AssertValidPosition(); + return true; + } + + [Conditional("DEBUG")] + private readonly void AssertValidPosition() { - _consumedAtStartOfCurrentSpan += _currentSpan.Length; // track consumed length - SequencePosition position; - if (!_sequence.IsSingleSegment) +#if DEBUG + Debug.Assert(_currentSpanIndex >= 0 && _currentSpanIndex <= _currentSpan.Length, "span index out of range"); + if (_currentSpanIndex == _currentSpan.Length) // should only be at-length for EOF { - position = new(_nextPositionObject, _nextPositionInteger); + ReadOnlySpan span = default; + SequencePosition position = Position; + Debug.Assert(!TryGetNextBuffer(in _sequence, ref position, ref span), "failed to eagerly read-ahead"); + } +#endif + } - while (_sequence.TryGetBuffer(position, out ReadOnlyMemory memory, out SequencePosition next)) + private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref SequencePosition position, ref ReadOnlySpan buffer) + { + SequencePosition end = sequence.End; + object? endObject = end.GetObject(); + + ReadOnlySequenceSegment? segment = position.GetObject() as ReadOnlySequenceSegment; + if (segment is not null) + { + while (!ReferenceEquals(segment, endObject) && (segment = segment!.Next) is not null) { - if (memory.Length > 0) + ReadOnlySpan span = segment.Memory.Span; + + if (ReferenceEquals(segment, endObject)) { - _currentSpan = memory.Span; - _currentSpanIndex = 0; - _currentPositionObject = position.GetObject(); - _currentPositionInteger = position.GetInteger(); - _nextPositionObject = next.GetObject(); - _nextPositionInteger = next.GetInteger(); + // the last segment ends early + span = span.Slice(0, end.GetInteger()); + } + + if (!span.IsEmpty) // skip empty segments + { + // note: only update position+buffer on success + position = new(segment, 0); + buffer = span; return true; } - position = next; } } - - // at EOF - position = _sequence.End; - _currentPositionObject = position.GetObject(); - _currentPositionInteger = position.GetInteger(); - _nextPositionObject = default; - _nextPositionInteger = default; - _currentSpan = default; - _currentSpanIndex = 0; - Unsafe.AsRef(in _length) = _consumedAtStartOfCurrentSpan; // since we know it, avoid later cost if Length accessed return false; } @@ -314,6 +340,7 @@ private bool GetNextSpan() [MethodImpl(MethodImplOptions.AggressiveInlining)] public void Advance(long count) { + AssertValidPosition(); const long TooBigOrNegative = unchecked((long)0xFFFFFFFF80000000); if ((count & TooBigOrNegative) == 0 && _currentSpan.Length - _currentSpanIndex > (int)count) { @@ -324,6 +351,7 @@ public void Advance(long count) // Can't satisfy from the current span AdvanceToNextSpan(count); } + AssertValidPosition(); } /// @@ -337,11 +365,15 @@ internal void AdvanceCurrentSpan(long count) _currentSpanIndex += (int)count; if (_currentSpanIndex == _currentSpan.Length) - GetNextSpan(); + { + TryGetNextSpan(); + } + AssertValidPosition(); } private void AdvanceToNextSpan(long count) { + AssertValidPosition(); if (count < 0) { ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count); @@ -364,7 +396,7 @@ private void AdvanceToNextSpan(long count) // always want to move to next span here, even // if we've consumed everything we want (to enforce // eager fetch) - if (!GetNextSpan() || count == 0) + if (!TryGetNextSpan() || count == 0) { break; } @@ -375,6 +407,7 @@ private void AdvanceToNextSpan(long count) // Not enough data left- adjust for where we actually ended and throw ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.count); } + AssertValidPosition(); } /// @@ -411,25 +444,20 @@ internal readonly bool TryCopyMultisegment(Span destination) if (Remaining < destination.Length) return false; - ReadOnlySpan firstSpan = UnreadSpan; - Debug.Assert(firstSpan.Length < destination.Length); - firstSpan.CopyTo(destination); - int copied = firstSpan.Length; + ReadOnlySpan currentSpan = UnreadSpan; + Debug.Assert(currentSpan.Length < destination.Length); + currentSpan.CopyTo(destination); + int copied = currentSpan.Length; - SequencePosition position = new(_nextPositionObject, _nextPositionInteger); - while (_sequence.TryGetBuffer(position, out ReadOnlyMemory nextSegment, out SequencePosition next)) + SequencePosition position = Position; + while (TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) { - position = next; - if (nextSegment.Length > 0) + int toCopy = Math.Min(currentSpan.Length, destination.Length - copied); + currentSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); + copied += toCopy; + if (copied >= destination.Length) { - ReadOnlySpan nextSpan = nextSegment.Span; - int toCopy = Math.Min(nextSpan.Length, destination.Length - copied); - nextSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); - copied += toCopy; - if (copied >= destination.Length) - { - break; - } + break; } } From e49b8633d58c31962704af64aa157e79da1562b2 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 13:15:10 +0000 Subject: [PATCH 15/23] simplify advance further --- .../System/Buffers/SequenceReader.Search.cs | 4 +-- .../src/System/Buffers/SequenceReader.cs | 29 ++++++++++--------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 9bdc2b257a8c6c..60543df2cfc358 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -813,7 +813,7 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) Debug.Assert(currentSpan.Length < next.Length); int fullLength = next.Length; - SequencePosition position = Position; + object? segment = _currentPositionObject; while (next.StartsWith(currentSpan)) { @@ -828,7 +828,7 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) } // Need to check the next segment - if (!TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) + if (!TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) { return false; // no more data } diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 33092d6bc92d47..2c56bb05097cef 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -171,10 +171,11 @@ public readonly bool TryPeek(long offset, out T value) else { long remainingOffset = offset - (_currentSpan.Length - _currentSpanIndex); - SequencePosition position = Position; + + object? segment = _currentPositionObject; ReadOnlySpan currentSpan = default; - while (TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) { if (remainingOffset >= currentSpan.Length) { @@ -276,15 +277,15 @@ private void ResetReader() private bool TryGetNextSpan() { int lastLength = _currentSpan.Length; - SequencePosition position = Position; - if (!TryGetNextBuffer(in _sequence, ref position, ref _currentSpan)) + object? segment = _currentPositionObject; + if (!TryGetNextBuffer(in _sequence, ref segment, ref _currentSpan)) { return false; } _consumedAtStartOfCurrentSpan += lastLength; // track consumed length _currentSpanIndex = 0; - _currentPositionObject = position.GetObject(); - _currentPositionInteger = position.GetInteger(); + _currentPositionObject = segment; + _currentPositionInteger = 0; // all non-first segments start at zero AssertValidPosition(); return true; @@ -298,18 +299,18 @@ private readonly void AssertValidPosition() if (_currentSpanIndex == _currentSpan.Length) // should only be at-length for EOF { ReadOnlySpan span = default; - SequencePosition position = Position; - Debug.Assert(!TryGetNextBuffer(in _sequence, ref position, ref span), "failed to eagerly read-ahead"); + object? segment = _currentPositionObject; + Debug.Assert(!TryGetNextBuffer(in _sequence, ref segment, ref span), "failed to eagerly read-ahead"); } #endif } - private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref SequencePosition position, ref ReadOnlySpan buffer) + private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer) { SequencePosition end = sequence.End; object? endObject = end.GetObject(); - ReadOnlySequenceSegment? segment = position.GetObject() as ReadOnlySequenceSegment; + ReadOnlySequenceSegment? segment = @object as ReadOnlySequenceSegment; if (segment is not null) { while (!ReferenceEquals(segment, endObject) && (segment = segment!.Next) is not null) @@ -324,8 +325,8 @@ private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref Sequen if (!span.IsEmpty) // skip empty segments { - // note: only update position+buffer on success - position = new(segment, 0); + // note: only update object+buffer on success + @object = segment; buffer = span; return true; } @@ -449,8 +450,8 @@ internal readonly bool TryCopyMultisegment(Span destination) currentSpan.CopyTo(destination); int copied = currentSpan.Length; - SequencePosition position = Position; - while (TryGetNextBuffer(in _sequence, ref position, ref currentSpan)) + object? segment = _currentPositionObject; + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) { int toCopy = Math.Min(currentSpan.Length, destination.Length - copied); currentSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); From 11a7b46f8ef81b8e7ce2b3a1054ce05b1427e011 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 14:34:36 +0000 Subject: [PATCH 16/23] remove debug-check from AssertValidPosition() to make the compiler happier --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 2c56bb05097cef..c23c0f9202b630 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -294,7 +294,6 @@ private bool TryGetNextSpan() [Conditional("DEBUG")] private readonly void AssertValidPosition() { -#if DEBUG Debug.Assert(_currentSpanIndex >= 0 && _currentSpanIndex <= _currentSpan.Length, "span index out of range"); if (_currentSpanIndex == _currentSpan.Length) // should only be at-length for EOF { @@ -302,7 +301,6 @@ private readonly void AssertValidPosition() object? segment = _currentPositionObject; Debug.Assert(!TryGetNextBuffer(in _sequence, ref segment, ref span), "failed to eagerly read-ahead"); } -#endif } private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer) From 570d2218cc973455acc0bc0defd7f8ee8aa925db Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 16:05:20 +0000 Subject: [PATCH 17/23] TryPeek/TryRead: avoid bounds check by hoisting into locals --- .../src/System/Buffers/SequenceReader.cs | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index c23c0f9202b630..251414048ad41e 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -128,13 +128,19 @@ public readonly long Length public readonly bool TryPeek(out T value) { AssertValidPosition(); - if (_currentSpanIndex == _currentSpan.Length) // only true at EOF due to eager read + + // hoisting into locals for the compare allows us to avoid a bounds check + int i = _currentSpanIndex; + ReadOnlySpan currentSpan = _currentSpan; + + if ((uint)i < (uint)currentSpan.Length) { - value = default; - return false; + value = currentSpan[i]; + return true; } - value = _currentSpan[_currentSpanIndex]; - return true; + + value = default; + return false; } /// @@ -201,22 +207,24 @@ public readonly bool TryPeek(long offset, out T value) public bool TryRead(out T value) { AssertValidPosition(); - switch (_currentSpan.Length - _currentSpanIndex) + + // hoisting into locals for the compare allows us to avoid a bounds check + int i = _currentSpanIndex; + ReadOnlySpan currentSpan = _currentSpan; + + if ((uint)i < (uint)currentSpan.Length) { - case 0: // end of current span; since we move ahead eagerly: EOF - value = default; - return false; - case 1: // one left in current span - value = _currentSpan[_currentSpanIndex++]; - TryGetNextSpan(); // move ahead eagerly - AssertValidPosition(); - return true; - default: - value = _currentSpan[_currentSpanIndex++]; - AssertValidPosition(); - return true; + value = currentSpan[i]; + if (++_currentSpanIndex == currentSpan.Length) + { + TryGetNextSpan(); // move ahead eagerly + } + AssertValidPosition(); + return true; } + value = default; + return false; } /// From c1c6d5921692a0c9032123fb33d6093510fafe2c Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 16:11:07 +0000 Subject: [PATCH 18/23] nit: use local for the addition too (cuts a `mov` from the JIT output) --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 251414048ad41e..50061f7bdbcc7b 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -216,7 +216,7 @@ public bool TryRead(out T value) { value = currentSpan[i]; - if (++_currentSpanIndex == currentSpan.Length) + if ((_currentSpanIndex = i + 1) == currentSpan.Length) { TryGetNextSpan(); // move ahead eagerly } From 594f879813cc67912fedcaaac5ccb1d74931b444 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Thu, 2 Mar 2023 19:06:44 +0000 Subject: [PATCH 19/23] test fixes --- .../System/Buffers/SequenceReader.Search.cs | 9 +++- .../src/System/Buffers/SequenceReader.cs | 42 +++++++++++-------- 2 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 60543df2cfc358..602b33da51b662 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -827,11 +827,16 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) return true; } + // move past the values we have validated + next = next.Slice(currentSpan.Length); + // Need to check the next segment - if (!TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) + if (!TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) { - return false; // no more data + // Nothing left + return false; } + if (currentSpan.Length > next.Length) { currentSpan = currentSpan.Slice(0, next.Length); diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 50061f7bdbcc7b..1e66259fad1fec 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -181,7 +181,7 @@ public readonly bool TryPeek(long offset, out T value) object? segment = _currentPositionObject; ReadOnlySpan currentSpan = default; - while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) { if (remainingOffset >= currentSpan.Length) { @@ -285,16 +285,24 @@ private void ResetReader() private bool TryGetNextSpan() { int lastLength = _currentSpan.Length; - object? segment = _currentPositionObject; - if (!TryGetNextBuffer(in _sequence, ref segment, ref _currentSpan)) + if (!TryGetNextBuffer(in _sequence, ref _currentPositionObject, ref _currentSpan, out bool advanced)) { + if (advanced) // we can still advance while returning false if the later segments are all empty + { + _currentPositionInteger = _currentSpanIndex = 0; + _consumedAtStartOfCurrentSpan += lastLength; // track consumed length + } + else + { + // make sure we position at the end of the last (current) segment + _currentSpanIndex = lastLength; + } + AssertValidPosition(); return false; } + Debug.Assert(advanced); + _currentPositionInteger = _currentSpanIndex = 0; _consumedAtStartOfCurrentSpan += lastLength; // track consumed length - _currentSpanIndex = 0; - _currentPositionObject = segment; - _currentPositionInteger = 0; // all non-first segments start at zero - AssertValidPosition(); return true; } @@ -307,33 +315,31 @@ private readonly void AssertValidPosition() { ReadOnlySpan span = default; object? segment = _currentPositionObject; - Debug.Assert(!TryGetNextBuffer(in _sequence, ref segment, ref span), "failed to eagerly read-ahead"); + Debug.Assert(!TryGetNextBuffer(in _sequence, ref segment, ref span, out _), "failed to eagerly read-ahead"); } } - private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer) + private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer, out bool advanced) { SequencePosition end = sequence.End; object? endObject = end.GetObject(); - + advanced = false; ReadOnlySequenceSegment? segment = @object as ReadOnlySequenceSegment; if (segment is not null) { - while (!ReferenceEquals(segment, endObject) && (segment = segment!.Next) is not null) + while (!ReferenceEquals(segment, endObject) && (@object = segment = segment!.Next) is not null) { - ReadOnlySpan span = segment.Memory.Span; + advanced = true; + buffer = segment!.Memory.Span; if (ReferenceEquals(segment, endObject)) { // the last segment ends early - span = span.Slice(0, end.GetInteger()); + buffer = buffer.Slice(0, end.GetInteger()); } - if (!span.IsEmpty) // skip empty segments + if (!buffer.IsEmpty) // skip empty segments { - // note: only update object+buffer on success - @object = segment; - buffer = span; return true; } } @@ -457,7 +463,7 @@ internal readonly bool TryCopyMultisegment(Span destination) int copied = currentSpan.Length; object? segment = _currentPositionObject; - while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan)) + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) { int toCopy = Math.Min(currentSpan.Length, destination.Length - copied); currentSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); From 892a9e71fcd033ded059bb1d2e4d744e7b634566 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 3 Mar 2023 11:21:34 +0000 Subject: [PATCH 20/23] Advance: don't short-circuit (we expect both sides to pass) --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 1e66259fad1fec..23981ac99609d7 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -355,8 +355,9 @@ public void Advance(long count) { AssertValidPosition(); const long TooBigOrNegative = unchecked((long)0xFFFFFFFF80000000); - if ((count & TooBigOrNegative) == 0 && _currentSpan.Length - _currentSpanIndex > (int)count) + if ((count & TooBigOrNegative) == 0 & _currentSpan.Length - _currentSpanIndex > (int)count) { + // ^^^ note & (rather than &&) is intentional; both sides can be safely evaluated _currentSpanIndex += (int)count; } else From eeb4bffa60cc5e59e594726c7df0f6e530e4027a Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Fri, 3 Mar 2023 16:06:19 +0000 Subject: [PATCH 21/23] work around JIT regressions --- .../System/Buffers/SequenceReader.Search.cs | 2 +- .../src/System/Buffers/SequenceReader.cs | 54 +++++++++++-------- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs index 602b33da51b662..8fb93907b62e9b 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.Search.cs @@ -831,7 +831,7 @@ private bool IsNextSlow(scoped ReadOnlySpan next, bool advancePast) next = next.Slice(currentSpan.Length); // Need to check the next segment - if (!TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) + if (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan) != TryGetNextBufferResult.SuccessHaveData) { // Nothing left return false; diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 23981ac99609d7..a0951ad0068704 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -181,7 +181,7 @@ public readonly bool TryPeek(long offset, out T value) object? segment = _currentPositionObject; ReadOnlySpan currentSpan = default; - while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan) == TryGetNextBufferResult.SuccessHaveData) { if (remainingOffset >= currentSpan.Length) { @@ -282,29 +282,30 @@ private void ResetReader() /// /// Get the next segment with available data, if any. /// + [MethodImpl(MethodImplOptions.NoInlining)] private bool TryGetNextSpan() { int lastLength = _currentSpan.Length; - if (!TryGetNextBuffer(in _sequence, ref _currentPositionObject, ref _currentSpan, out bool advanced)) + switch (TryGetNextBuffer(in _sequence, ref _currentPositionObject, ref _currentSpan)) { - if (advanced) // we can still advance while returning false if the later segments are all empty - { + case TryGetNextBufferResult.SuccessHaveData: _currentPositionInteger = _currentSpanIndex = 0; _consumedAtStartOfCurrentSpan += lastLength; // track consumed length - } - else - { - // make sure we position at the end of the last (current) segment + AssertValidPosition(); + return true; + case TryGetNextBufferResult.FailureAllRemainingSegmentsEmpty: + // means we advanced, so we still need to reset some things + _currentPositionInteger = _currentSpanIndex = 0; + _consumedAtStartOfCurrentSpan += lastLength; // track consumed length + break; + default: // no more segments + // make sure we position at the end of the last (current) segment, + // even if we didn't change segment _currentSpanIndex = lastLength; - } - AssertValidPosition(); - return false; + break; } - Debug.Assert(advanced); - _currentPositionInteger = _currentSpanIndex = 0; - _consumedAtStartOfCurrentSpan += lastLength; // track consumed length AssertValidPosition(); - return true; + return false; } [Conditional("DEBUG")] @@ -315,21 +316,28 @@ private readonly void AssertValidPosition() { ReadOnlySpan span = default; object? segment = _currentPositionObject; - Debug.Assert(!TryGetNextBuffer(in _sequence, ref segment, ref span, out _), "failed to eagerly read-ahead"); + Debug.Assert(TryGetNextBuffer(in _sequence, ref segment, ref span) == TryGetNextBufferResult.FailureNoMoreSegments, "failed to eagerly read-ahead"); } } - private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer, out bool advanced) + private enum TryGetNextBufferResult + { + SuccessHaveData = 0, + FailureNoMoreSegments = 1, + FailureAllRemainingSegmentsEmpty = 2, + } + + private static TryGetNextBufferResult TryGetNextBuffer(in ReadOnlySequence sequence, ref object? @object, ref ReadOnlySpan buffer) { SequencePosition end = sequence.End; object? endObject = end.GetObject(); - advanced = false; + TryGetNextBufferResult result = TryGetNextBufferResult.FailureNoMoreSegments; ReadOnlySequenceSegment? segment = @object as ReadOnlySequenceSegment; if (segment is not null) { while (!ReferenceEquals(segment, endObject) && (@object = segment = segment!.Next) is not null) { - advanced = true; + result = TryGetNextBufferResult.FailureAllRemainingSegmentsEmpty; // until we know otherwise buffer = segment!.Memory.Span; if (ReferenceEquals(segment, endObject)) @@ -340,11 +348,12 @@ private static bool TryGetNextBuffer(in ReadOnlySequence sequence, ref object if (!buffer.IsEmpty) // skip empty segments { - return true; + result = TryGetNextBufferResult.SuccessHaveData; + break; } } } - return false; + return result; } /// @@ -385,6 +394,7 @@ internal void AdvanceCurrentSpan(long count) AssertValidPosition(); } + [MethodImpl(MethodImplOptions.NoInlining)] // avoid what looks like a JIT regression with it inlining this too much from Advance private void AdvanceToNextSpan(long count) { AssertValidPosition(); @@ -464,7 +474,7 @@ internal readonly bool TryCopyMultisegment(Span destination) int copied = currentSpan.Length; object? segment = _currentPositionObject; - while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan, out _)) + while (TryGetNextBuffer(in _sequence, ref segment, ref currentSpan) == TryGetNextBufferResult.SuccessHaveData) { int toCopy = Math.Min(currentSpan.Length, destination.Length - copied); currentSpan.Slice(0, toCopy).CopyTo(destination.Slice(copied)); From 0412ab1c0a63117fe31736546ee44cea544379f6 Mon Sep 17 00:00:00 2001 From: Marc Gravell Date: Tue, 7 Mar 2023 12:18:42 +0000 Subject: [PATCH 22/23] nit: TryGetNextBuffer - defer updating result; empty segments are v.rare --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index a0951ad0068704..1ef8cbb9a253bd 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -337,7 +337,6 @@ private static TryGetNextBufferResult TryGetNextBuffer(in ReadOnlySequence se { while (!ReferenceEquals(segment, endObject) && (@object = segment = segment!.Next) is not null) { - result = TryGetNextBufferResult.FailureAllRemainingSegmentsEmpty; // until we know otherwise buffer = segment!.Memory.Span; if (ReferenceEquals(segment, endObject)) @@ -351,6 +350,9 @@ private static TryGetNextBufferResult TryGetNextBuffer(in ReadOnlySequence se result = TryGetNextBufferResult.SuccessHaveData; break; } + + // tell the caller that we *had* more segments (and have advanced), even if not useful + result = TryGetNextBufferResult.FailureAllRemainingSegmentsEmpty; } } return result; @@ -394,7 +396,7 @@ internal void AdvanceCurrentSpan(long count) AssertValidPosition(); } - [MethodImpl(MethodImplOptions.NoInlining)] // avoid what looks like a JIT regression with it inlining this too much from Advance + [MethodImpl(MethodImplOptions.NoInlining)] // avoid inlining this too much from Advance private void AdvanceToNextSpan(long count) { AssertValidPosition(); From e82567979e91ca1072f6509f0e5b865d531ebf33 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 17 Aug 2023 10:14:06 -0500 Subject: [PATCH 23/23] Fix two rebase errors --- .../System.Memory/src/System/Buffers/SequenceReader.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs index 1ef8cbb9a253bd..86c8552430160c 100644 --- a/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs +++ b/src/libraries/System.Memory/src/System/Buffers/SequenceReader.cs @@ -246,8 +246,6 @@ public void Rewind(long count) return; } - Consumed -= count; - if (_currentSpanIndex >= count) { _currentSpanIndex -= (int)count; @@ -276,7 +274,7 @@ private void ResetReader() this = new(_sequence); // reinstate the length - Unsafe.AsRef(_length) = length; + Unsafe.AsRef(in _length) = length; } ///