Prepared System.Buffer.Primitives for API Review #1993

KrzysztofCwalina · 2017-12-22T21:02:33Z

Unified Positions
Moved ReadCursorOperations around. These APIs are not general purpose Position APIs. They only work with ReadOnlyBuffer.
Renamed ReadOnlyBufferReader to BufferReader
Simplified namespaces

@pakrym, @davidfowl, @joshfree, @terrajobst

benaadams · 2017-12-22T21:10:56Z

src/System.Buffers.Experimental/System/Buffers/BufferExtensions.cs

@@ -56,7 +56,7 @@ public static int CopyTo(this IMemoryList<byte> list, Span<byte> destination)
            {
                var current = list.Memory.Span;
                var index = current.IndexOf(value);
-                if (index != -1) return Collections.Sequences.Position.Create(list, index);
+                if (index != -1) return new Collections.Sequences.Position(list, index);


System.Collections.Sequences is imported, is there a name clash?

i.e. can this be just Position?

Ah, forgot about it. I will remove.

pakrym · 2017-12-22T21:33:15Z

tests/System.Buffers.Primitives.Tests/ReadableBufferFacts.cs

@@ -180,5 +181,129 @@ public void Create_WorksWithOwnedMemory()
            b => b.Slice(0, 70).Slice(0, b.End),
            b => b.Slice(70, b.Start)
        };
+
+        static bool TryGetBuffer(Position begin, Position end, out ReadOnlyMemory<byte> data, out Position next)


Why is this here?

I can move the tests that call Seek to Pipelines.Tests and then eliminate it. What do you think?

These tests were testing internal implementations of ROB why would we want to move them to Pipelines? Or even worse duplicate code being tested inside the test itself.

What do you propose I do? Is there a way to test this without calling ROB's private APIs?

Make them internal. It has been done in other places to verify implementation details.

pakrym · 2017-12-22T21:33:56Z

tests/System.IO.Pipelines.Tests/TestBufferFactory.cs

+using System.Linq;
+using System.Text;
+
+namespace System.IO.Pipelines.Tests


This is useful not on only for pipelines, why is it moved back?

It's used just by pipelines tests

No, see https://github.com/dotnet/corefxlab/blob/master/tests/System.Buffers.Primitives.Tests/ReadableBufferFacts.cs#L21

Not sure why it builds but it's used by ROB and RBR tests that are in primitives.

There are two copies. I will remove one of them (i.e. the one in pipelines)

pakrym · 2017-12-22T21:34:52Z

src/System.Buffers.Primitives/System/Collections/Position.cs

+                    return segment;
+            }
+
+            //ThrowHelper.ThrowInvalidOperationException(ExceptionResource.UnexpectedSegmentType);


Why remove throwing?

I have no idea. Good catch.

pakrym · 2017-12-22T21:38:30Z

src/System.IO.Pipelines/System/IO/Pipelines/PositionOperations.cs

@@ -8,7 +8,7 @@

 namespace System.IO.Pipelines
 {
-    public static class ReadCursorOperations
+    public static class PositionOperations


Why is it moved back to pipelines?

Well, it's pipelines specific. It does not work with an arbitrary position. It only works with positions of IMemoryList, and possibly other things that ReadOnlyBuffer supports.

It works with everything that ReadOnlyBuffer supports that's why it's in buffers. There is no need to have another copy in Pipelines.

Position is more general than Buffers. Look in System.Collections.Sequences. There is a Hashtable and ArrayList implementation that uses it.

Then move these methods to be on ROB

Hmmm, I have done it initially and it did not work. But now it does. I will fix.

I removed it and added InternalsVisible from Pipelines.Tests to ReadOnlyBuffer. We need to design proper seek APIs though.

Wait, why do you need InternalsVisisbleTo to Pipelines.Tests? Just don't move SeekTests.cs from primitives.

pakrym

Too many copies of TryGetBuffer around.

pakrym · 2017-12-22T21:56:28Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer_helpers.cs

+    public readonly partial struct ReadOnlyBuffer 
+    {
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        static bool TryGetBuffer(Position begin, Position end, out ReadOnlyMemory<byte> data, out Position next)


Why internal? It can be private

The same thing, there is a test in tests/System.Buffers.Primitives.Tests/ReadableBufferFacts.cs using it.

pakrym · 2017-12-22T21:56:39Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer_helpers.cs

+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        static Position Seek(Position begin, Position end, long bytes, bool checkEndReachable = true)


same: why not private?

So the tests from tests/System.Buffers.Primitives.Tests/ReadableBufferFacts.cs do not require copied code or being moved.

pakrym · 2017-12-22T21:56:59Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer_helpers.cs

+        }
+
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        static Position SeekMultiSegment(Position begin, Position end, long bytes, bool checkEndReachable)


private for everything else

pakrym · 2017-12-22T22:28:34Z

src/System.Buffers.Primitives/System/Collections/Position.cs

+        public static explicit operator int(Position position) => position._index;
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public (T segment, int index) Get<T>()


Do we expect consumers to check if the segment is non-null/default?

Depends on what they use the position for.

I think in most cases they either use it or not. If they don't use it, direct access to Index is enough. If they need both then I prefer throwing to returning default.

Sure, we can play with this. But I still want to do an experiment with changing the check to typeof(T) == typeof(Segment). I will try to do it after the new year. If if does not work, we can remove this method.

pakrym · 2017-12-22T22:29:49Z

src/System.Buffers.Primitives/System/Collections/Position.cs

+        public static bool operator ==(Position left, Position right) => left._index == right._index && left._segment == right._segment;
+        public static bool operator !=(Position left, Position right) => left._index != right._index || left._segment != right._segment;
+
+        public static Position operator +(Position value, int index)


I thought we were going to limit the ability to modify positions?

We now have public ctors, Index, and Segment. The + operator is much more hidden than those.
Also, without the operator, code that uses position as index is very wacky. It does not care about the Segment, yet it has to deal with it (in the ctor, at least).

We can think about it more, but I would treat this as a separate PR.

pakrym · 2017-12-22T22:30:33Z

src/System.IO.Pipelines.Extensions/ReadableBufferSequence.cs

@@ -16,16 +15,16 @@ public ReadableBufferSequence(ReadOnlyBuffer buffer) : this()
            _buffer = buffer;
        }

-        public Collections.Sequences.Position First => Collections.Sequences.Position.Create(_buffer.Start.Segment, _buffer.Start.Index);
+        public Position Start => new Position(_buffer.Start.Segment, _buffer.Start.Index);


Why have start at all if we treat default as a Start?

I dont think we treat default as start. I remove this early this week.

pakrym · 2017-12-22T22:48:03Z

src/System.Buffers.Primitives/System/Buffers/ReadOnlyBuffer_helpers.cs

@@ -10,8 +10,65 @@ namespace System.Buffers
 {
    public readonly partial struct ReadOnlyBuffer 
    {
+        public static int Seek(Position begin, Position end, out Position result, byte byte0)


Change them to be extension/instance methods on ReadOnlyBuffer

Let's do it separately. I think I have seen some code that calls the APIs with positions other that the buffer's start/end

I think Seek is the last one, but we can do it in a separate pass, just don't like moving things back and forth.

* Unified Positions * Removed fully qualified Position names * Added accidentally removed throw * Removed one copy of TestBufferFactory * Rearrange Seek Methods

Unified Positions

db8e98e

benaadams reviewed Dec 22, 2017

View reviewed changes

Removed fully qualified Position names

3390b8b

KrzysztofCwalina force-pushed the PositionUnification branch from 2f0d92d to 3390b8b Compare December 22, 2017 21:17

pakrym reviewed Dec 22, 2017

View reviewed changes

pakrym suggested changes Dec 22, 2017

View reviewed changes

Added accidentally removed throw

1714380

pakrym reviewed Dec 22, 2017

View reviewed changes

Removed one copy of TestBufferFactory

7941960

pakrym reviewed Dec 22, 2017

View reviewed changes

Rearrange Seek Methods

fd2f095

pakrym reviewed Dec 22, 2017

View reviewed changes

pakrym approved these changes Dec 22, 2017

View reviewed changes

KrzysztofCwalina merged commit 252c882 into dotnet:master Dec 22, 2017

KrzysztofCwalina deleted the PositionUnification branch January 3, 2018 00:20

Prepared System.Buffer.Primitives for API Review #1993

Prepared System.Buffer.Primitives for API Review #1993

Conversation

KrzysztofCwalina commented Dec 22, 2017

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

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

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 left a comment • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym left a comment •

edited

Loading