Created performance tests for ROS First and TryGet methods #28760

AlexRadch · 2018-04-03T09:23:45Z

I created performance tests for ROS First and TryGet methods. Also I suggest more clear and fast code for TryGetBuffer and GetFirstBuffer methods.

…suggest more clear and fast code for TryGetBuffer method.

ahsonkhan · 2018-04-03T20:30:18Z

@AlexRadch, going forward, please make sure to label your PRs with "area-System.Memory" if you are modifying the code in this assembly. It notifies the area owners and makes relevant issues/PRs easier to sort and find. Thanks!

ahsonkhan · 2018-04-03T20:31:45Z

I created performance tests for ReadOnlySequence.TryGet method.

Can you please share the results of the new performance tests that were added? Before vs after? See example comparison data like dotnet/coreclr#17370 / dotnet/coreclr#17391

ahsonkhan · 2018-04-03T20:36:07Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-            int endIndex = 0;
+            GetTypeAndIndices(start.GetInteger(), end.GetInteger(), out SequenceType type, out int startIndex, out int endIndex);
            object startObject = start.GetObject();
-            if (startObject != null)


Why don't we need to check startObject != null anymore?

For the last segment TryGet method return true and return Position with GetObject() equal to null. It is used as flag that all segments was processed. So when TryGet called with Position with GetObject() == nul means that all segments was processed and we return false. It is only one condition to return false and stop loop processing.

pakrym · 2018-04-03T20:36:12Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.TryGet.cs

+                int localInt = 0;
+                using (iteration.StartMeasurement())
+                {
+                    for (int i = 0; i < Benchmark.InnerIterationCount; i++)


Why two loops over Benchmark.InnerIterationCount?

One loop was too fast. I added second loop.

If a single loop is too fast, increase the value of InnerCount (private const int InnerCount = 1000;)

pakrym · 2018-04-03T20:37:22Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+                {
+                    for (int i = 0; i < Benchmark.InnerIterationCount; i++)
+                    {
+                        for (int j = 0; j < size / 10; j++)


Why size/10?

MemoryManager is slow so I make less loops to reduce test time.

In that case, change the value of Benchmark.InnerIterationCount for this test.

[Benchmark(InnerIterationCount = InnerCount / 10)]

ahsonkhan · 2018-04-03T20:37:24Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-            object endObject = end.GetObject();
-
-            if (type != SequenceType.MultiSegment && type != SequenceType.Empty && startObject != endObject)
-                ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached();


Now that we removed this check, do we still use this ThrowInvalidOperationException_EndPositionNotReached anywhere? If not, can we remove it from ThrowHelper?

ThrowInvalidOperationException_EndPositionNotReached is used in CheckEndReachable method also. This method is slow and I will remove it later when I will create performance tests for GetPosition and Slice methods and I will suggest more performance code for Seek method.

pakrym · 2018-04-03T20:38:29Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+        [InlineData(1000, 100, 800)]
+        [InlineData(1000 * 1000, 0, 1000 * 1000)]
+        [InlineData(1000 * 1000, 1000, 998 * 1000)]
+        private static void Byte_SingleSegment(int size, int start, int lenght)


SingleSegment -> FullArray, others are single segments too.

Others uses direct casting to different objects (array, string or MemoryManager). Single segment uses casting to BufferSegment. It is perfomance tests for different internal cases.

ahsonkhan · 2018-04-03T20:39:01Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+// See the LICENSE file in the project root for more information.
+
+using System.Memory.Tests;
+using System.MemoryTests;


What helpers/etc are we using from this namespace?

We using BufferSegment and CustomMemoryForTest from usual tests.

pakrym · 2018-04-03T20:39:29Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+        }
+
+        [Benchmark(InnerIterationCount = InnerCount)]
+        [InlineData(1000, 0, 1000)]


Are all these modifications useful? Do they show different use cases/performance?

I will remove duplicates

pakrym · 2018-04-03T20:48:00Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-            int length = endIndex - startIndex;
-            object endObject = end.GetObject();
-
-            if (type != SequenceType.MultiSegment && type != SequenceType.Empty && startObject != endObject)


Why was this check removed?

It slow down performance and moved to Debug.Asset(startObject == endObject)

ahsonkhan · 2018-04-03T21:41:16Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+        [InlineData(1000, 100, 800)]
+        [InlineData(1000 * 1000, 0, 1000 * 1000)]
+        [InlineData(1000 * 1000, 1000, 998 * 1000)]
+        private static void Byte_Array(int size, int start, int lenght)


typo: lenght, here and elsewhere

ahsonkhan · 2018-04-03T21:42:42Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+        [InlineData(1000, 100, 800)]
+        [InlineData(1000 * 1000, 0, 1000 * 1000)]
+        [InlineData(1000 * 1000, 1000, 998 * 1000)]
+        private static void Byte_Memory(int size, int start, int lenght)


I don't think there is much utility in a performance test like this one. It should be identical to the Byte_Array test.

It is test for ROS with MemoryManager inside.

Is there any difference in performance between the Byte_Array and Byte_Memory tests?

Run the performance tests and share the results. In the System.Memory/tests/Performance directory, run this command:
msbuild /t:BuildAndTest /p:Performance=true /p:ConfigurationGroup=Release /p:TargetOS=Windows_NT

See https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/performance-tests.md#building-and-running-tests

Is there any difference in performance between the Byte_Array and Byte_Memory tests?

~ 2.5 times slow.

ahsonkhan · 2018-04-03T21:44:49Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.TryGet.cs

+                {
+                    for (int i = 0; i < Benchmark.InnerIterationCount; i++)
+                    {
+                        for (int j = 0; j < Benchmark.InnerIterationCount; j++)


As @pakrym mentioned, we shouldn't have two loops going over InnerIterationCount (here and elsewhere)

AlexRadch · 2018-04-04T07:50:01Z

I updated tests and created logs files for old and new code. How to create comparison diagram? Is there some tools?

AlexRadch · 2018-04-04T09:30:34Z

I updated performance tests to remove code issues. Performance results are next:

pakrym · 2018-04-04T15:12:09Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

            {
                Debug.Assert(startObject is ReadOnlySequenceSegment<T>);
                ReadOnlySequenceSegment<T> startSegment = Unsafe.As<ReadOnlySequenceSegment<T>>(startObject);
+


What revert?

empty newlines

pakrym · 2018-04-04T15:12:20Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-                {
-                    length = memory.Length - startIndex;
-                }
+                if (startObject != end.GetObject())


don't remove braces

pakrym · 2018-04-04T15:15:49Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

                memory = new ReadOnlyMemory<T>(Unsafe.As<T[]>(startObject));
            }
-            else if (type == SequenceType.MemoryManager)
+            else if (typeof(T) == typeof(char) && type == SequenceType.String)


Why reorder?

Because this section can be optimized by compiler if T is not char.
The last section is optimized by code (comparition is commented).
So now 1 or 2 sections can be optimized.
If makes this section last then only one section will be optimized always.

pakrym · 2018-04-04T17:09:53Z

How stable are perf results? Any idea why multisegment became slower? (That's the case we care about the most)

ahsonkhan · 2018-04-06T03:48:06Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.TryGet.cs

+                {
+                    for (int i = 0; i < Benchmark.InnerIterationCount; i++)
+                    {
+                        var p = buffer.Start;


nit: use of var

ahsonkhan · 2018-04-06T03:48:18Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.TryGet.cs

+                {
+                    for (int i = 0; i < Benchmark.InnerIterationCount; i++)
+                    {
+                        var p = buffer.Start;


nit: same here.

AlexRadch · 2018-04-06T06:07:37Z

Spend two days to make code more faster. Now performance results are next:

ahsonkhan · 2018-04-06T07:01:37Z

src/System.Memory/src/System/Buffers/ReadOnlySequence.cs

+        {
+            // We take high order bits of two indexes index and move them
+            // to a first and second position to convert to BufferType
+            return _sequenceStart.GetObject() == null ? SequenceType.Empty : (SequenceType)((((uint)_sequenceStart.GetInteger() & ReadOnlySequence.FlagBitMask) >> 30) | (uint)_sequenceEnd.GetInteger() >> 31);


This will be faster:

return _sequenceStart.GetObject() == null ? SequenceType.Empty : (SequenceType)(-(2 * (_sequenceStart.GetInteger() >> 31) + (_sequenceEnd.GetInteger() >> 31)));

I see two strange xor at left. But it is compiler! At right I see many hacks with negative values. Is it correct?

(((uint)_sequenceStart.GetInteger() & ReadOnlySequence.FlagBitMask) >> 30) | (uint)_sequenceEnd.GetInteger() >> 31
and
(-(2 * (_sequenceStart.GetInteger() >> 31) + (_sequenceEnd.GetInteger() >> 31))
are equivalent for all [start, end] between int.MinValue and int.MaxValue.

Here is the breakdown:

It is works https://dotnetfiddle.net/kJjenR

ahsonkhan · 2018-04-06T07:04:30Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-            if (type != SequenceType.MultiSegment && type != SequenceType.Empty && startObject != endObject)
-                ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached();
+            int endIndex = GetIndex(_sequenceEnd);
+            //int length = GetIndex(_sequenceEnd) - positionIndex;


remove commented out code

ahsonkhan · 2018-04-06T07:06:18Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs


-                data = startSegment.Memory;
+                // Bounds check
+                #if DEBUG


These checks need to be in Release as well (since we need to throw an exception that would be visible to the user if they passed in invalid input).

Old code does not make correct Bounds checks. Do make bounds check in production code? It will slow down performance.

Should we check in production next ?

Debug.Assert(positionObject is ReadOnlySequenceSegment<T>);

Debug.Assert(startSegment.RunningIndex - positionSegment.RunningIndex <= GetIndex(position) - GetIndex(_sequenceStart) || GetIndex(position) - GetIndex(_sequenceEnd) <= endSegment.RunningIndex - positionSegment.RunningIndex);

Debug.Assert(nextSegment != null);

Debug.Assert(positionObject == _sequenceEnd.GetObject())

Debug.Assert(GetIndex(_sequenceStart) <= GetIndex(position) && GetIndex(position) <= GetIndex(_sequenceEnd));

ahsonkhan · 2018-04-06T07:08:48Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-                    if (nextSegment == null && startSegment != endObject)
-                        ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached();
+                    ReadOnlySequenceSegment<T> nextSegment = positionSegment.Next;
+                    Debug.Assert(nextSegment != null);


This check cannot be in debug only.

The tests shouldn't be passing. There is a test gap here. We should have tests verifying that we throw if if (nextSegment == null && nextSegment != _sequenceEnd.GetObject())

cc @pakrym

Here (nextSegment == null && nextSegment != _sequenceEnd.GetObject()) second part is always true because _sequenceEnd.GetObject() is never == null

ahsonkhan · 2018-04-06T07:09:32Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

                    next = new SequencePosition(nextSegment, 0);
-                    length = data.Length - startIndex;
+                    endIndex = memory.Length;
+                    //length = memory.Length - positionIndex;


remove commented out code

ahsonkhan · 2018-04-06T07:19:40Z

src/System.Memory/src/System/Buffers/ReadOnlySequence.cs

+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private SequenceType GetSequenceType()
+        {
+            // We take high order bits of two indexes index and move them


Typo in the comment: ...two indexes index...

I copied it from other method ;)

ahsonkhan · 2018-04-06T07:53:33Z

src/System.Memory/tests/Performance/Perf.ReadOnlySequence.First.cs

+
+namespace System.Buffers.Tests
+{
+    public class Rerf_ReadOnlySequence_First


Typo: Rerf_ReadOnlySequence_First -> Perf... (same with TryGet)

ahsonkhan · 2018-04-06T09:25:43Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-            else if (typeof(T) == typeof(char) && type == SequenceType.String)
-            {
-                Debug.Assert(startObject is string);
+                    memory = new ReadOnlyMemory<T>(Unsafe.As<T[]>(positionObject));


We don't need Unsafe code here and we can afford to take the performance hit of casting it (to get type safety).

If we re-write the TryGetBuffer method as follows, we get type safety, and we are still faster than the original:

[MethodImpl(MethodImplOptions.AggressiveInlining)] private bool TryGetBuffer(in SequencePosition position, out ReadOnlyMemory<T> memory, out SequencePosition next) { object positionObject = position.GetObject(); next = default; if (positionObject == null) { memory = default; return false; } SequenceType type = GetSequenceType(position); object endObject = _sequenceEnd.GetObject(); int startIndex = GetIndex(position); int endIndex = GetIndex(_sequenceEnd); if (type == SequenceType.MultiSegment) { Debug.Assert(positionObject is ReadOnlySequenceSegment<T>); //ReadOnlySequenceSegment<T> startSegment = Unsafe.As<ReadOnlySequenceSegment<T>>(positionObject); ReadOnlySequenceSegment<T> startSegment = (ReadOnlySequenceSegment<T>)positionObject; if (startSegment != endObject) { ReadOnlySequenceSegment<T> nextSegment = startSegment.Next; if (nextSegment == null) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); next = new SequencePosition(nextSegment, 0); memory = startSegment.Memory.Slice(startIndex); } else { memory = startSegment.Memory.Slice(startIndex, endIndex - startIndex); } } else if (type == SequenceType.Array) { if (positionObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(positionObject is T[]); //memory = new ReadOnlyMemory<T>(Unsafe.As<T[]>(positionObject), startIndex, endIndex - startIndex); memory = new ReadOnlyMemory<T>((T[])positionObject, startIndex, endIndex - startIndex); } else if (typeof(T) == typeof(char) && type == SequenceType.String) { if (positionObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(positionObject is string); //memory = (ReadOnlyMemory<T>)(object)(Unsafe.As<string>(positionObject)).AsMemory(startIndex, endIndex - startIndex); memory = (ReadOnlyMemory<T>)(object)((string)positionObject).AsMemory(startIndex, endIndex - startIndex); } else // type == SequenceType.MemoryManager { if (positionObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(type == SequenceType.MemoryManager); Debug.Assert(positionObject is MemoryManager<T>); //memory = Unsafe.As<MemoryManager<T>>(positionObject).Memory.Slice(startIndex, endIndex - startIndex); memory = ((MemoryManager<T>)positionObject).Memory.Slice(startIndex, endIndex - startIndex); } return true; }

cc @GrabYourPitchforks, @jkotas, @davidfowl

If we check that positionObject == endObject means that unsafe code will be correct always.

In single segment we can just ignore positionObject and get memory directly from endObject with unsafe code.

If we check that positionObject == endObject means that unsafe code will be correct always.

That is true. Good point.

In single segment we can just ignore positionObject and get memory directly from endObject with unsafe code.

I am not completely convinced of this. Some tests will boost my confidence in that.

FYI, @stephentoub, brought up an interesting issue the unsafe type casting can cause due to struct tearing: https://github.com/dotnet/corefx/issues/28920

ahsonkhan · 2018-04-06T09:31:25Z

src/System.Memory/src/System/Buffers/ReadOnlySequence.cs

-            sequenceType = Start.GetObject() == null ? SequenceType.Empty : (SequenceType)((((uint)Start.GetInteger() >> 30) & 2) | (uint)End.GetInteger() >> 31);
+            // We take high order bits of two indexes and move them
+            // to a first and second position to convert to SequenceType
+            sequenceType = _sequenceStart.GetObject() == null


Call the GetSequenceType instead, to avoid code duplication
sequenceType = GetSequenceType();

ahsonkhan · 2018-04-06T09:32:32Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-                ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached();
+            int endIndex = GetIndex(_sequenceEnd);

+            SequenceType type = GetSequenceType();


Shouldn't GetSequenceType() take position as a parameter here instead of using _sequenceStart?

Shouldn't GetSequenceType() take position as a parameter here instead of using _sequenceStart?

Sequance type is stored as sing bits in _sequenceStart.GetInteger() and _sequenceEnd.GetInteger() except default (_sequenceStart.GetObject == null).

I get that, but there is no guarantee that the SequencePosition that the user passes in would have the same sign bits as _sequenceStart.

You can't validate the type by one sequence position. It might be .Start or .End so it might or might not have the flag.

I get that, but there is no guarantee that the SequencePosition that the user passes in would have the same sign bits as _sequenceStart.

We ignore position flag because it can contain any flag, from Start or from End or does not contain flag at all.
We story flags in _sequenceStart.GetInteger() and _sequenceEnd.GetInteger(), so we get flags from _sequenceStart.GetInteger() and _sequenceEnd.GetInteger().

ahsonkhan · 2018-04-06T10:00:11Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs


        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal ReadOnlyMemory<T> GetFirstBuffer(in SequencePosition start, in SequencePosition end)
+        private ReadOnlyMemory<T> GetFirstBuffer()


Try something like this:

[MethodImpl(MethodImplOptions.AggressiveInlining)] private ReadOnlyMemory<T> GetFirstBuffer() { object startObject = _sequenceStart.GetObject(); if (startObject == null) { return default; } SequenceType type = GetSequenceType(); object endObject = _sequenceEnd.GetObject(); int startIndex = GetIndex(_sequenceStart); int endIndex = GetIndex(_sequenceEnd); if (type == SequenceType.MultiSegment) { Debug.Assert(startObject is ReadOnlySequenceSegment<T>); ReadOnlySequenceSegment<T> startSegment = Unsafe.As<ReadOnlySequenceSegment<T>>(startObject); if (startObject != endObject) { return startSegment.Memory.Slice(startIndex); } return startSegment.Memory.Slice(startIndex, endIndex - startIndex); } if (type == SequenceType.Array) { if (startObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(startObject is T[]); return new ReadOnlyMemory<T>(Unsafe.As<T[]>(startObject), startIndex, endIndex - startIndex); } if (typeof(T) == typeof(char) && type == SequenceType.String) { if (startObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(startObject is string); return (ReadOnlyMemory<T>)(object)(Unsafe.As<string>(startObject)).AsMemory(startIndex, endIndex - startIndex); } Debug.Assert(type == SequenceType.MemoryManager); if (startObject != endObject) ThrowHelper.ThrowInvalidOperationException_EndPositionNotReached(); Debug.Assert(type == SequenceType.MemoryManager); Debug.Assert(startObject is MemoryManager<T>); return Unsafe.As<MemoryManager<T>>(startObject).Memory.Slice(startIndex, endIndex - startIndex); }

[MethodImpl(MethodImplOptions.AggressiveInlining)] private SequenceType GetSequenceType() { return (SequenceType)(-(2 * (_sequenceStart.GetInteger() >> 31) + (_sequenceEnd.GetInteger() >> 31))); } [MethodImpl(MethodImplOptions.AggressiveInlining)] private SequenceType GetSequenceType(in SequencePosition position) { return (SequenceType)(-(2 * (position.GetInteger() >> 31) + (_sequenceEnd.GetInteger() >> 31))); }

jkotas · 2018-04-06T13:42:14Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

-                Debug.Assert(startObject is ReadOnlySequenceSegment<T>);
-                ReadOnlySequenceSegment<T> startSegment = Unsafe.As<ReadOnlySequenceSegment<T>>(startObject);
+                Debug.Assert(positionObject is ReadOnlySequenceSegment<T>);
+                ReadOnlySequenceSegment<T> positionSegment = Unsafe.As<ReadOnlySequenceSegment<T>>(positionObject);


I do not think we should be using Unsafe.As here at all, for the same reason why we are not using Unsafe.As in System.Memory<T>. I think that using Unsafe.As here is striking a wrong balance between security and performance.

ahsonkhan · 2018-04-06T19:35:58Z

@AlexRadch, can you please split up the performance tests and the optimizations into separate PRs?

Let's get the performance tests checked in (since they are not contentious) and then discuss the optimizations separately.

I would recommend updating this PR to only include the performance tests that you added. Take the perf optimizations and recommendations and open up a new PR. Thanks!

AlexRadch · 2018-04-07T05:17:51Z

I added all @ahsonkhan optimizations and all bounds checks. The main delay is made by a safe type conversion in a line https://github.com/dotnet/corefx/pull/28760/files#diff-6a3b2bb4fe535f3ac47622b764877747R66
This safe type conversion slows method performance about 2 times.

pakrym · 2018-04-07T17:30:08Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

+                }

+                // Start or Middle segment
+                {


Don't create blocks without flow control statements.

Why? I want local variables.

Using these blocks to create separate scopes makes the code harder to read and maintain (especially with local variable name re-use and gotos). This method is already tricky as is so I would try to avoid additional complexity. I would recommend simplifying it. Also, I wouldn't use the trusted flag either (those checks should always happen).

Goto used to throw exception or exit. So it did not make method more complex to understand.

ahsonkhan · 2018-04-08T00:59:05Z

src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs

    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal bool TryGetBuffer(in SequencePosition start, in SequencePosition end, out ReadOnlyMemory<T> data, out SequencePosition next)
+        internal bool TryGetBuffer(in SequencePosition position, out ReadOnlyMemory<T> memory, out SequencePosition next, bool tructed = false)


typo: tructed (also I would get rid of this and always check/throw).

also I would get rid of this and always check/throw

Trusted true is called from internal tested code where position is always correct. So all checks will not throw any exception. As result this method will be about two times faster on multi segments for internal methods.

public TryGet method use this method with trusted = false.

ahsonkhan · 2018-04-08T01:06:08Z

@AlexRadch, the changes in this PR are getting harder to review and keep up with, especially with some iterations of code churn. I suggest isolating the change (and limiting it to only what is essential) rather than re-working it too much.

My suggestion:

Split this PR and only include the performance tests (which can then be merged relatively quickly, to establish a baseline).
Open a separate PR for the optimization and focus on a single method (say TryGetBuffer) and aim to simplify the logic so it easy to read and understand. Do not introduce substantial code complexity to try to get slight performance improvements at the micro-benchmark scale (the reverse trade-off would be great).
Based on the feedback and work there (on the 2nd PR), apply similar technique in a 3rd PR for the other method (say GetFirstBuffer).

Otherwise, getting anything merged would take too long and the code review becomes cumbersome.

Thanks a lot for your efforts and contribution on this! If you prefer, I could take the changes you made (and the intent of this PR), starting at your branch, and take it forward on your behalf by submitting a PR on top of your contribution. Let me know if you would prefer that.

AlexRadch · 2018-04-08T06:15:11Z

@ahsonkhan I am making such changes but I do not finish Slice and others perf tests yet. I will finish all perf tests and then publish them. I see all messages about this and working on it.

AlexRadch added 2 commits April 3, 2018 05:36

Created performance tests for ReadOnlySequence.First property.

b300dc3

Created performance tests for ReadOnlySequence.TryGet method. Also I …

de3b73d

…suggest more clear and fast code for TryGetBuffer method.

ahsonkhan added the area-System.Memory label Apr 3, 2018

ahsonkhan requested review from benaadams and pakrym April 3, 2018 20:28

ahsonkhan reviewed Apr 3, 2018

View reviewed changes

pakrym reviewed Apr 3, 2018

View reviewed changes

ahsonkhan reviewed Apr 3, 2018

View reviewed changes

pakrym reviewed Apr 3, 2018

View reviewed changes

ahsonkhan reviewed Apr 3, 2018

View reviewed changes

pakrym reviewed Apr 3, 2018

View reviewed changes

ahsonkhan reviewed Apr 3, 2018

View reviewed changes

ahsonkhan mentioned this pull request Apr 4, 2018

Created performance tests for ReadOnlySequence.First property #28741

Closed

ahsonkhan changed the title ~~Created performance tests for ReadOnlySequence.TryGet method~~ Created performance tests for ReadOnlySequence.TryGetBuffer and GetFirstBuffer Apr 4, 2018

AlexRadch changed the title ~~Created performance tests for ReadOnlySequence.TryGetBuffer and GetFirstBuffer~~ Created performance tests for ROS First and TryGet methods Apr 4, 2018

AlexRadch added 2 commits April 4, 2018 15:01

Updated performance tests

1a81d00

Restore csproj file

70817a2

pakrym reviewed Apr 4, 2018

View reviewed changes

Review issues

90bec07

ahsonkhan reviewed Apr 6, 2018

View reviewed changes

AlexRadch added 2 commits April 6, 2018 11:18

Maked a little faster

1eb1737

Maked a little faster

a2fe830

ahsonkhan modified the milestones: 2.2.0, 2.1.0 Apr 6, 2018

ahsonkhan reviewed Apr 6, 2018

View reviewed changes

Review issues

a24228d

ahsonkhan reviewed Apr 6, 2018

View reviewed changes

jkotas reviewed Apr 6, 2018

View reviewed changes

AlexRadch added 2 commits April 7, 2018 11:38

Added bounds check

f2cf784

Small performance optimization

2766450

Added TryGetTrusted

a279085

pakrym reviewed Apr 7, 2018

View reviewed changes

ahsonkhan reviewed Apr 8, 2018

View reviewed changes

AlexRadch closed this Apr 8, 2018

Created performance tests for ROS First and TryGet methods #28760

Created performance tests for ROS First and TryGet methods #28760

Uh oh!

Conversation

AlexRadch commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan commented Apr 3, 2018

Uh oh!

ahsonkhan commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakrym Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexRadch commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexRadch commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexRadch Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

AlexRadch commented Apr 3, 2018 •

edited

Loading

ahsonkhan commented Apr 3, 2018 •

edited

Loading

pakrym Apr 3, 2018 •

edited

Loading

ahsonkhan Apr 3, 2018 •

edited

Loading

AlexRadch commented Apr 4, 2018 •

edited

Loading

AlexRadch Apr 4, 2018 •

edited

Loading