Merge pull request #69 from dotnet/sequencereader-2

SequenceReader<T>: Round 2

2018/System.Buffers.SequenceReader/README.md

@@ -5,7 +5,9 @@ Status: **Needs more work** |
 [Issue](https://github.com/dotnet/corefx/issues/32588) |
 [Video](https://www.youtube.com/watch?v=0WN1OXKBMl8)
 
-## Notes
+## Session 1
+
+### Notes
 
 * Two types are being proposed here:
     - `SequenceReader<T>`: wraps a `ReadOnlySequence<T>`
@@ -27,7 +29,7 @@ Status: **Needs more work** |
 * None of this is accessible to languages that do not support `ref structs`,
   such as Visual Basic.
 
-## API feedback
+### API feedback
 
 * `Rewind` seems a bit weird
 * Can we unify `Advance` and `Rewind` into `Seek()` where the offset can be
@@ -38,7 +40,7 @@ Status: **Needs more work** |
   cost of zeroing out the passed in scratch buffer as it's rarely used. Counter
   argument from @stephentoub:
   > If `Peek` returned the number of `T`s actually peeked, why would the span
-  > need to be zero'd out beforehand? (It looks like it's currently defined to
+  > need to be zeroed out beforehand? (It looks like it's currently defined to
   > return a `ReadOnlySpan<T>`, and presumably that logically incorporates that
   > by being of the correct length?)
 * `IsNext` is a bit weird. Maybe we should we use `StartsWith` and
@@ -49,3 +51,72 @@ Status: **Needs more work** |
   - We shouldn't name them `Advance` as they could conflict with the existing
     `Advance(long)` method when `T` is `long`.
 
+## Session 2
+
+### SequenceReader\<T>
+
+* Should we remove the `unamanged` constrained and replace with a `.ctor` check?
+    - Done for pinning and being able to take a pointer?
+* Can we have some sample code that uses a `ReadOnlySpan<T>`-based reader vs. a
+  `ReadOnlySequence<T>`-based reader?
+    - We can then compare the per gains
+    - We can then compare the usability gains
+    - From [@stephentoub](https://github.com/stephentoub):
+    > And specifically (since I'm not sure I was clear in the meeting), here's
+    > what I was hoping to see. Given some common scenario that reads from /
+    > parses data from a pipeline:
+    > 
+    > * Use the BufferReader that only supports span, and have a single method
+    >   (written as we would in production code as best we can) with all of the
+    >   relevant boilerplate that handles interaction both with the pipeline and
+    >   with managing sequences/calls to the span-based reader.
+    > * Use the BufferReader that supports sequence, and have a single method
+    >   (written as we would in production code as best we can) with all of the
+    >   relevant boilerplate that handles interaction both with the pipeline and
+    >   with the sequence-based reader.
+    > 
+    > I'd like to us to be able to look at the code the provides the same
+    > functionality and the benchmarks on it and compare side-by-side both
+    > usability and performance. If we could do the test as part of kestrel
+    > (e.g. two different runnable commits off of master) and look at plaintext
+    > benchmarks and what the kestrel code looks like both ways, even better.
+    > 
+    > If the version with the span-based reader is totally incomprehensible and
+    > provides only modest gains, then that leads us one way. If the version
+    > with the span-based reader is only mildly more cumbersome and provides
+    > significant gains, then that leads us another way. Etc. I just want to
+    > make sure we're all in agreement on the goals, seeing the same data, and
+    > then coming to a joint conclusion.
+* `AtLastSegment`. The logic outlined in the comment is wrong, you need to honor
+  position, and not check whether there is next segment.
+* `Allocator` seems odd. If we need it, should we promote the delegate or move
+  it elsewhere? We can't use a `Func` due to the generic instantiation issue
+  with `Span<T>`.
+    - Should the allocator be passed in as argument? Storing it as a field might
+      cause additional memory barrier
+* `AreAnyNext` -> `IsNextAny`
+* Suggestion from last time was to rename `IsNext` to `StartsWith` but the
+  latter isn't clear whether it's relative to the current position vs. the
+  buffer's start.
+* We may want to make `advancePastDelimiter` different methods to avoid the perf
+  issue due to the extra branches.
+* The struct has to be passed by `ref` or you have bugs. Also, since it's large,
+  passing it by value would also tank perf.
+
+### SequenceReaderExtensions
+
+* All methods have a limit of reading 128 `T`s
+    - This allows stack allocated buffers to be used and seemed rationale
+    - Might be too small for specialized scenarios, such as many leading zeros
+    - We cannot really expose APIs with sizes as this would likely destroy perf
+* No APIs can produce strings, we should probably add that as dealing with
+  things going across buffers is hard and we don't want folks to go through an
+  intermediary array.
+
+## BufferExtensions
+
+* Can't we make this an instance method?
+
+## UnsafeGetPositionWithinSegment
+
+* Move to `SequenceMarshal`