Removed Old BufferReader #2025

KrzysztofCwalina · 2018-01-05T00:48:38Z

I changed all members of the old reader into extension methods over the pipelines reader, and deleted the old reader.

pakrym · 2018-01-05T00:59:56Z

src/System.Buffers.Experimental/System/Buffers/BufferReader_search.cs

+                bytes = default;
+                return false;
+            }
+            bytes = new ReadOnlyBuffer(reader.Position, position.Value);


Still not correct.

as we discussed, we need to make it correct, or remove the ctor.

pakrym · 2018-01-05T01:00:45Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

@@ -42,7 +44,9 @@ public BufferReader(TSequence buffer)

        public Position Position => _currentPosition + _index;

-        public ReadOnlySpan<byte> Span => _currentSpan;
+        public ReadOnlySpan<byte> CurrentSegment => _currentSpan;


Same comment about Span. We don't call it Segment in IMemoryList why call it segment here.

Cause IMemoryList is a list of Memory<T> instances. ReadOnlyBuffer is a sequence of segments, not sequence of spans.

It's a list of things you can get Memory from, same as here, list of things you can get Span from.

I think we should call it Span

pakrym · 2018-01-05T01:02:07Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

+            _index = 0;
+        }
+
+        public Position? PositionOf(byte value)


There is same logic in ROB.Seek

Yeah, but I don't have access to End

That doesn't mean we have to duplicate everything on reader now.

Agreed. We should keep this API minimal.

I want to eliminate the old reader. To do this I need a method roughly like the following:

public static bool TryReadUntill(ref this BufferReader<ReadOnlyBuffer> reader, out ReadOnlyBuffer bytes, byte delimiter) { var position = reader.PositionOf(delimiter); if (position == null) { bytes = default; return false; } bytes = new ReadOnlyBuffer(reader.Position, position.Value); reader.SkipTo(position.Value); reader.Skip(1); return true; }

Even if I change the this parameter to reader over ROB, I am not sure how to implement it. Can you suggest a way to implement it without adding PositionOf to the reader?

var start = reader.Position; while (!reader.IsEnd || reader.Read() != delimiter) {} return new ROB(start, reader.Position);

The method is a Try, but Read has side effects.

Copy reader to local and copy back if needed.

I think it's ok as a workaround for missing APIs, but it's not a good long term solution. The current implementation is super fast: does not copy large structs, uses vectorized IndexOf to find matches, in common cases where the match is found in the first segment, it simply adjusts the _index (once). Your implementation (workaround) is not even in the same ballpark.

I know, I'm not claiming that it is efficient. But we have fast one in ROB and we need to decide how to reuse it and avoid code duplication.

pakrym · 2018-01-05T01:02:53Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

+            }
+        }
+
+        public static int CopyTo(BufferReader<TSequence> bytes, Span<byte> buffer)


There is same logic on ROB. Lets not duplicate it.

Yeah, but I don't have access to ROB. I agree that it would be good to somehow refactor to reuse the logic. Any ideas for how to refactor?

We decided some time ago that we won't try to implement everything on top of ISequence and now we are trying to do it.

We should expose Sequence and so allow access to all these implementations on ROB. We can expand ISequence in the future to accommodate new features but we shouldn't go into rabbit hole right now. Especially considering these implementations are not used in production code and may not be thoroughly tested.

We can expose SliceBefore() and SliceAfter() extension methods in the way I talked in other thread so the consumer can get ROB of either and call existing methods on it.

I am not sure how it helps here. I don't have BufferReader<ROB>; I have BufferReader<TSequence>

As I said, in a previous meeting we decided not to try building everything on top of ISequence, especially while we have only one real implementation. We can live with supporting these things only on ROB and see what other things would implement ISequence<ReadOnlyMemory<byte>> if there would be any other at all.

Maybe moving the reader to ISequence was a mistake too.

We have several problems in various abstractions: ISequence, Position, ROB. But I would not throw baby with the bathwater. I implemented many extensions over the APIs we have now (the no merge PR) and it seems to be almost working. I think we need to solve a couple of small problems to enable both reading over sequences and slicing. Let's chat more about it today.

davidfowl · 2018-01-05T03:56:05Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

-        public ReadOnlySpan<byte> Span => _currentSpan;
+        public ReadOnlySpan<byte> CurrentSegment => _currentSpan;
+
+        public ReadOnlySpan<byte> UnreadSegment => _currentSpan.Slice(_index);


This is a useful property 😄 . I think we should expose this as the RemainingSpan and we should also have RemainingBytes

Remaining sounds like you are talking about entire buffer, especially when RemainingBytes is absolute.

I think in general calling these two concepts "Span" has the same issue: it's not clear what the span contains. Segment is very clear: the type is a list of segments and it's one of these.

davidfowl · 2018-01-05T03:57:22Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

+            return default;
+        }
+
+        public Position? PositionOf(ReadOnlySpan<byte> value)


Can we remove this for now? I'd like to keep this API minimal and fully tested. Expose whatever needs to be exposed to implement this in the experimental APIs

I don't think it can be removed, and I don't think we can sweep the issue under the rug. The remaining abstraction is not powerful enough to implement reading of variable length data (based on searches). We would have to expose many fields of the reader (in addition to the sequence) to implement for example ReadUntill, at which point using the reader would be very error prone as the callers could mess up its state too easily.

pakrym · 2018-01-05T17:12:07Z

I think moving reader to use ISequence and treating it as an abstraction was a mistake, we should concentrate our time on finishing things that around ROB, not trying to add more stuff that supports ISequence especially considering we have exactly one implementation of it.

We may also want to consider not shipping interface at all until we find a real need and at least another implementer for it because at the current state it clearly does not allow all the scenarios we are trying to use it for (lack of Slice is one of the bigger problems).

KrzysztofCwalina · 2018-01-05T21:47:21Z

OK, I removed all but Skip(Position) and CopyTo. Are we good with these two?

pakrym · 2018-01-05T22:02:14Z

They both exist in ROB in forms of Slice and CopyTo, why duplicate?

pakrym · 2018-01-05T22:07:14Z

There are also no tests for them.

KrzysztofCwalina · 2018-01-08T16:12:21Z

They both exist in ROB in forms of Slice and CopyTo, why duplicate?

Because I believe we cannot develop hundreds of BCL APIs operating over ROB; it's too policy heavy. I think we need a ROB abstraction. There are many ways to design a multi-segment buffer and ROB is just one of them. In the BCL we don't like to take dependencies on one particular design (unless the design is super simple). Whenever we do, we endup having to redesign and reimplement these APIs operating over such policy heavy types every other release.

There are also no tests for them.

Thus "NO MERGE". I said in the PR message that I will add tests once we agree on the APIs.

KrzysztofCwalina · 2018-01-08T18:32:50Z

OK, I added tests to CopyTo, the only method we agreed to keep on the reader.

pakrym · 2018-01-08T18:36:35Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer.cs

@@ -17,6 +17,8 @@ namespace System.Buffers
        internal readonly Position BufferStart;
        internal readonly Position BufferEnd;

+        public static readonly ReadOnlyBuffer Empty = new ReadOnlyBuffer(new byte[0]);


Array.Empty<>?

Not available in NS 1.1

pakrym · 2018-01-08T18:37:52Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

    {
        public static BufferReader<TSequence> Create<TSequence>(TSequence buffer) where TSequence : ISequence<ReadOnlyMemory<byte>>
        {
            return new BufferReader<TSequence>(buffer);
        }
+
+        public static int CopyTo<TSequence>(BufferReader<TSequence> reader, Span<byte> destination)


Why do you need it here and not on the instance?

It's a limitation of the C# compiler. The Span can be stack allocated but the instance CopyTo method could store the passed in span into the _currentSegment field. See dotnet/roslyn#23433. It's quite unfortunate though.

cc: @VSadov, @jaredpar

pakrym · 2018-01-08T18:41:21Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

            {
                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.length);
            }
        }
+
+        internal static int CopyTo(BufferReader<TSequence> bytes, Span<byte> destination)


It doesn't follow usual CopyTo conventions about destination sizes. Maybe we should name it differently? Read(span)?

It does not advance the index, so it cannot be called Read, but I will rename it to Peek

Maybe it should advance? Wouldn't it be the most common use case?

Or TryRead(Span).

KrzysztofCwalina · 2018-01-08T20:32:49Z

@dotnet-bot test Innerloop OSX10.12 Debug Build and Test

KrzysztofCwalina · 2018-01-08T20:33:08Z

@dotnet-bot test Innerloop Ubuntu16.04 Debug Build and Test

pakrym · 2018-01-08T22:30:57Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBufferReader.cs

        private TSequence _sequence;
        private Position _currentPosition;
        private Position _nextPosition;
+
        private int _consumedBytes;


KrzysztofCwalina added the * NO MERGE * label Jan 5, 2018

pakrym reviewed Jan 5, 2018

View reviewed changes

davidfowl reviewed Jan 5, 2018

View reviewed changes

KrzysztofCwalina force-pushed the OldReaderRemoved branch from c7d4442 to 1e2fba4 Compare January 5, 2018 17:14

KrzysztofCwalina added 2 commits January 5, 2018 11:04

Unified Readers

10d32ad

Removed PositionOf from reader

c0fe750

KrzysztofCwalina force-pushed the OldReaderRemoved branch from 1e2fba4 to c0fe750 Compare January 5, 2018 21:39

KrzysztofCwalina added 4 commits January 8, 2018 08:53

f

dbc3731

Added tests for CopyTo

b5a6c32

f

cea68ff

f

be2903b

KrzysztofCwalina removed the * NO MERGE * label Jan 8, 2018

KrzysztofCwalina changed the title ~~Removed Old BufferReader [DO NOT MERGE]~~ Removed Old BufferReader Jan 8, 2018

pakrym reviewed Jan 8, 2018

View reviewed changes

KrzysztofCwalina added 2 commits January 8, 2018 10:53

reacted to changes to ReadOnlyBuffer

b60c068

Renamed CopyTo to Peek

bf02173

pakrym approved these changes Jan 8, 2018

View reviewed changes

KrzysztofCwalina added 3 commits January 8, 2018 12:45

f

d929bc8

f

f849402

f

6320664

pakrym reviewed Jan 8, 2018

View reviewed changes

KrzysztofCwalina added 9 commits January 8, 2018 14:32

f

ab77b30

f

08fceaf

f

6356eda

f

d4c0a03

f

6f5214a

f

21b0f25

f

71b7c2c

Merge branch 'master' into OldReaderRemoved

8ab777a

Merge branch 'master' into OldReaderRemoved

126a3a5

KrzysztofCwalina merged commit 1fecfe7 into dotnet:master Jan 9, 2018

KrzysztofCwalina deleted the OldReaderRemoved branch January 10, 2018 01:08

Removed Old BufferReader #2025

Removed Old BufferReader #2025

Conversation

KrzysztofCwalina commented Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Jan 5, 2018 • edited Loading

KrzysztofCwalina commented Jan 5, 2018

pakrym commented Jan 5, 2018

pakrym commented Jan 5, 2018

KrzysztofCwalina commented Jan 8, 2018 • edited Loading

KrzysztofCwalina commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina commented Jan 8, 2018

KrzysztofCwalina commented Jan 8, 2018

Choose a reason for hiding this comment

KrzysztofCwalina commented Jan 5, 2018 •

edited

Loading

KrzysztofCwalina Jan 5, 2018 •

edited

Loading

KrzysztofCwalina Jan 5, 2018 •

edited

Loading

pakrym Jan 5, 2018 •

edited

Loading

KrzysztofCwalina Jan 5, 2018 •

edited

Loading

KrzysztofCwalina Jan 5, 2018 •

edited

Loading

pakrym commented Jan 5, 2018 •

edited

Loading

KrzysztofCwalina commented Jan 8, 2018 •

edited

Loading

KrzysztofCwalina Jan 8, 2018 •

edited

Loading