Improve default handling in ReadOnlySequence.Slice #38431

pgovind · 2019-06-10T23:30:42Z

Fixes https://github.com/dotnet/corefx/issues/35254

The null dereference was because we now allow nullable objects in SequencePosition. Once I fixed that, there's still the issue of what default(SequencePosition)._object should be. If the expectation is that the default start position is 0, it makes sense that default(SequencePosition)._object = Start?

Also, this is my first patch in corefx, so I'd appreciate comments on the new test and its location. Also, this seems like a small change, but how do folks generally measure the performance impacts of new changes?

src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Default.cs

ahsonkhan · 2019-06-11T01:04:28Z

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

@@ -455,7 +455,7 @@ public ReadOnlySequence<T> Slice(SequencePosition start, SequencePosition end)
        public ReadOnlySequence<T> Slice(SequencePosition start)
        {
            BoundsCheck(start);
-            return SliceImpl(start);
+            return SliceImpl(start.Equals(default) ? Start : start);


As far as I recall, this code is quite perf-sensitive. Are we OK with introducing branches with checks like these?

Maybe start.GetObject() == null instead?

It's probably not worth it, but consider even calling a SliceImpl helper that takes object/int (and passing in _startObject, _startInteger rather than creating a SequencePosition to pass in which gets deconstructed anyway).

cc @davidfowl, @pakrym

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

ahsonkhan · 2019-06-11T01:06:56Z

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

@@ -355,7 +355,7 @@ private void BoundsCheck(in SequencePosition position)
                // Storing this in a local since it is used twice within InRange()
                ulong startRange = (ulong)(((ReadOnlySequenceSegment<T>)startObject!).RunningIndex + startIndex);
                if (!InRange(
-                    (ulong)(((ReadOnlySequenceSegment<T>)position.GetObject()!).RunningIndex + sliceStartIndex),
+                    (ulong)((position.GetObject() != null ? ((ReadOnlySequenceSegment<T>)position.GetObject()!).RunningIndex : 0) + sliceStartIndex),


This seems counter to the annotated bang (!) on position.GetObject(). @safern - what should be the correct annotation here? Should we remove the !?

nit: We may want to extract this out in a local to help with readability.

The compiler will not do deep data flow analysis here to track that you already checked for null on the call of GetObject() and that you're calling it again when it is not null. So unfortunately the ! is still needed here. I just validated that: https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEBMBGAsAKFwYgDsBXAG1ImFLgAI5DLrcUBmG9GgYRoG9caB7NgEtCMGgFkAFABEaABwD2UYTGGLCASl79BASBQB2BctXrCAOgDicGAHlgAKzgBjGFO0BCALw0S5GgB+GikpAC1NJRU1DWtbB2c3D00LAElCABM4AA8aEBoABgBuXQEAX1wKvBxWdjQaOT4cPVKaRSdXGGCAfXbE8V9/UhKcQRpW2r7O4Jt7DqTtbwA+Gl75mBGq5jYOMJ1m2tFxdKzcnhoAc1simihrmjKaXwxNoA

If perf is not affected and adding an extra local doesn't hurt, that would be the only way to remove the !, but if that hurts the produced IL we should leave as is.

cc: @cston I guess this behavior is by design and not something the compiler can infer?

@safern, yes, this is by design - the compiler tracks the nullability of locals, parameters, and fields and properties of those only.

ahsonkhan · 2019-06-11T01:14:34Z

Also, this is my first patch in corefx, so I'd appreciate comments on the new test and its location. Also, this seems like a small change, but how do folks generally measure the performance impacts of new changes?

The test location seems fine to me.

For perf testing, I would start by looking at the existing perf tests which are measuring just the API call specifically (do a before/after comparison). If there are some tests missing, please add them:
https://github.com/dotnet/performance/blob/master/src/benchmarks/micro/corefx/System.Buffers/ReadOnlySequenceTests.cs

For more general, end-to-end testing and impact, you would have to take a look at where such APIs are used most heavily, which in this case is in Kestrel Web Server (this is probably not necessary for such a change). For instance:
https://github.com/aspnet/AspNetCore/blob/a3c8bd16f7c0eaa473364460e0354b2b0cc7fdad/src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs#L54
https://github.com/aspnet/AspNetCore/blob/04bf1bf32e13d35d4d7f61f9cbeb9bfa22e0c617/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs#L47

cc @jkotalik, who might also be able to help with that (if needed)

pgovind · 2019-06-12T01:00:03Z

Edit: Patch is complete now. Can be reviewed

Just adding a note here: the patch is not complete yet. There’s bugs in the other Slice APIs that I’m fixing. I’ll have a complete patch up later tonight or tomorrow morning

ahsonkhan

Otherwise, LGTM (pending some perf numbers, even from micro-benchmarks)

src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Default.cs

ahsonkhan · 2019-06-22T21:24:11Z

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

@@ -455,7 +455,7 @@ public ReadOnlySequence<T> Slice(SequencePosition start, SequencePosition end)
        public ReadOnlySequence<T> Slice(SequencePosition start)
        {
            BoundsCheck(start);
-            return SliceImpl(start);
+            return SliceImpl(start.GetObject() == null ? Start : start);


It looks like we are already checking whether start.GetObject() is null in BoundsCheck:

long runningIndex = ((ReadOnlySequenceSegment<T>?)position.GetObject())?.RunningIndex ?? 0;

But there isn't a great way to avoid that (and only occurs in one of the branches).

Maybe, store the result of start.Object == null in a bool and pass that in to BoundsCheck. Then runningIndex can be 0 unless that bool is true (in which case we can get rid of the null checks there). Something to consider, if it can reduce the instruction size of BoundsCheck and Slice.

long runningIndex = 0; if (positionIsNotNull) { Debug.Assert(position.GetObject() != null); runningIndex = ((ReadOnlySequenceSegment<Byte>)position.GetObject()).RunningIndex; }

sharplab.io

Store start.GetObject() != null in a local and re-use in the SliceImpl call

Right. In the works :)

ahsonkhan · 2019-06-22T21:25:05Z

src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Default.cs

@@ -122,6 +122,39 @@ public void Default_SliceNegativeLength()
            Assert.Throws<ArgumentOutOfRangeException>("length", () => buffer.Slice(buffer.Start, -1L));
        }

+        [Fact]


Can you share the code coverage numbers. Let's make sure all the new branches are covered (and if not, add more tests).

How do people generally share this? Can I just upload the result of the code coverage run here? In any case, this patch adds 1 new branch and code coverage is telling me that both branches are covered by the tests.

Yep, uploading the zip containing the code coverage works.

Looks like we have other branches too. Like in Slice(long start, SequencePosition end) and also the new if (positionIsNotNull) branch in BoundsCheck

Welp, it looks like github doesn't like zips

"\scratch2\scratch\prgovi\ROS_Slice_CodeCoverage.zip"

ahsonkhan · 2019-06-22T21:26:50Z

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

@@ -451,34 +454,25 @@ private SequenceType GetSequenceType()
        private static int GetIndex(int Integer) => Integer & ReadOnlySequence.IndexBitMask;

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private ReadOnlySequence<T> SliceImpl(in SequencePosition start, in SequencePosition end)
+        private ReadOnlySequence<T> SliceImpl(in object? startObject, in int startIndex, in object? endObject, in int endIndex)


From a quick view of the disassembly (in a similar sample), this pattern seems to help reduce instruction count. So looks good.

sharplab.io

From a quick view of the disassembly (in a similar sample), this pattern seems to help reduce instruction count.

On Linux, too?

Perf test?

@pgovind was going to do some perf validation.

Right, I thought this would've performed better too, but it does not and I don't have an explanation for why yet. It seems that calling SliceImpl(new SequencePosition(someObject, someIndex), someLength) is somehow faster than calling SliceImpl(someObject, someIndex, endObject, endLength) 🤷‍♂️.

Edit: I modified the patch to include just the bug fix and no refactoring.

On Linux, too?

@stephentoub, why do you think the behavior would be different on Linux?

why do you think the behavior would be different on Linux?

Different calling convention?

summary: better: 1, geomean: 1.203 worse: 1, geomean: 1.121 total diff: 2 | Slower | diff/base | Base Median (ns) | Diff Median (ns) | Modality| | ----------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:| | MicroBenchmarks.corefx.System.Memory.ReadOnlySequenceBenchmarks.StartPosition | 1.12 | 9.69 | 10.87 | | | Faster | base/diff | Base Median (ns) | Diff Median (ns) | Modality| | -------------------------------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:| | MicroBenchmarks.corefx.System.Memory.ReadOnlySequenceBenchmarks.StartPosition_An | 1.20 | 5.74 | 4.77 | |

pgovind · 2019-06-26T01:12:39Z

Updated with the latest numbers:

I setup some benchmarks here:

dotnet/performance#589

The results are as follows:
C:\Users\prgovi\Desktop\Work\performance\src\tools\ResultsComparer>dotnet run --base "C:\Users\prgovi\Desktop\Work\Benchmarks\Before" --diff "C:\Users\prgovi\Desktop\Work\Benchmarks\After" --threshold 2%
summary:
better: 5, geomean: 1.057
worse: 3, geomean: 1.152
total diff: 8

Slower	diff/base	Base Median (ns)	Diff Median (ns)
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.MS_StartPosition	1.24	8.44	10.47
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.StartPosition	1.17	4.97	5.79
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.MS_StartPosition_And_EndPositi	1.06	11.02	11.65

Faster	base/diff	Base Median (ns)	Diff Median (ns)
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.RepeatSlice_StartPosition_And_	1.08	24.39	22.56
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.Start_And_EndPosition	1.07	6.55	6.10
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.StartPosition_And_EndPosition	1.07	5.99	5.61
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.RepeatSlice	1.03	66.89	64.63
MicroBenchmarks.corefx.ReadOnlySequenceBenchmarks.MS_RepeatSlice	1.03	81.18	79.00

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

ahsonkhan · 2019-06-26T03:55:03Z

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

@@ -329,7 +329,7 @@ public ReadOnlySequence<T> Slice(long start, SequencePosition end)
        FoundInFirstSegment:
            // startIndex + start <= int.MaxValue
            Debug.Assert(start <= int.MaxValue - startIndex);
-            return SliceImpl(new SequencePosition(startObject, (int)startIndex + (int)start), end);
+            return SliceImpl(new SequencePosition(startObject, (int)startIndex + (int)start), new SequencePosition(sliceEndObject, (int)sliceEndIndex));


Aren't we potentially mismatching the object and the index here? sliceEndObject could be pointing to _startObject, but the sliceEndIndex is still the index from end

Nope. When sliceEndObject is null(happens only when default is passed in), sliceEndIndex will be 0.

As discussed offline, this issue needs to be resolved.

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

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

pgovind · 2019-06-26T17:35:36Z

Assuming the CI finishes before snapping, can I merge this patch? Are we ok with the perf numbers?

ghost · 2019-06-26T18:35:37Z

Hello @ahsonkhan!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

pgovind · 2019-06-26T21:52:25Z

src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Default.cs

+            Assert.Equal(0, slicedSequence.Length);
+
+            // Slice(x, default) returns empty if x = 0. Otherwise throws
+            sequence = sequence.Slice(2);


This should cover both the Slice(int, SequencePosition) and the Slice(SequencePosition, int) cases

How so? I would feel more confident if we had separate test cases for both overloads, explicitly.

We discussed this offline, and it makes sense to me now.

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

davidfowl · 2019-06-29T12:54:57Z

We'll look out for regressions.

* Improve default handling in ReadOnlySequence.Slice Commit migrated from dotnet/corefx@5f6cba8

pgovind requested review from ahsonkhan, stephentoub and danmoseley June 10, 2019 23:30

pgovind added the area-System.Memory label Jun 10, 2019

pgovind added this to the 3.0 milestone Jun 10, 2019

ahsonkhan reviewed Jun 11, 2019

View reviewed changes

pgovind force-pushed the ros_slice branch 2 times, most recently from ba77d2d to 0a9071b Compare June 14, 2019 19:48

ahsonkhan approved these changes Jun 22, 2019

View reviewed changes

ahsonkhan reviewed Jun 22, 2019

View reviewed changes

pgovind force-pushed the ros_slice branch from 0a9071b to b95b353 Compare June 24, 2019 22:39

Prashanth Govindarajan added 6 commits June 25, 2019 16:38

Improve default handling in ReadOnlySequence.Slice

cdb95da

Address feedback

e20784a

sq

e2a5e16

Bool and new SequencePosition

7636310

Delete the new SliceImpl method

95908ae

pgovind mentioned this pull request Jun 26, 2019

Benchmarks for ReadOnlySequence.Slice. dotnet/performance#589

Merged

ahsonkhan reviewed Jun 26, 2019

View reviewed changes

Address more PR feedback

053461e

pgovind force-pushed the ros_slice branch from 90f1325 to 053461e Compare June 26, 2019 17:26

sq

0980b98

ahsonkhan approved these changes Jun 26, 2019

View reviewed changes

ahsonkhan added the auto-merge Automatically merge PR once CI passes. label Jun 26, 2019

ahsonkhan removed the auto-merge Automatically merge PR once CI passes. label Jun 26, 2019

Bug fix

092169f

pgovind commented Jun 26, 2019

View reviewed changes

stephentoub reviewed Jun 26, 2019

View reviewed changes

src/System.Memory/src/System/Buffers/ReadOnlySequence.Helpers.cs Outdated Show resolved Hide resolved

sq

98c88a7

pgovind merged commit 5f6cba8 into master Jun 28, 2019

pgovind deleted the ros_slice branch June 28, 2019 16:42

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Improve default handling in ReadOnlySequence.Slice (dotnet/corefx#38431)

e078def

* Improve default handling in ReadOnlySequence.Slice Commit migrated from dotnet/corefx@5f6cba8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve default handling in ReadOnlySequence.Slice #38431

Improve default handling in ReadOnlySequence.Slice #38431

pgovind commented Jun 10, 2019 •

edited

Loading

ahsonkhan Jun 11, 2019 •

edited

Loading

ahsonkhan Jun 11, 2019

safern Jun 11, 2019

cston Jun 11, 2019

ahsonkhan commented Jun 11, 2019

pgovind commented Jun 12, 2019 •

edited

Loading

ahsonkhan left a comment

ahsonkhan Jun 22, 2019

ahsonkhan Jun 25, 2019

pgovind Jun 25, 2019

ahsonkhan Jun 22, 2019

pgovind Jun 24, 2019 •

edited

Loading

ahsonkhan Jun 25, 2019 •

edited

Loading

pgovind Jun 26, 2019

pgovind Jun 26, 2019

ahsonkhan Jun 22, 2019

stephentoub Jun 22, 2019

ahsonkhan Jun 24, 2019

pgovind Jun 25, 2019 •

edited

Loading

ahsonkhan Jun 25, 2019

stephentoub Jun 25, 2019

pgovind commented Jun 26, 2019 •

edited

Loading

ahsonkhan Jun 26, 2019

pgovind Jun 26, 2019

ahsonkhan Jun 26, 2019

pgovind commented Jun 26, 2019

ghost commented Jun 26, 2019

pgovind Jun 26, 2019

ahsonkhan Jun 27, 2019

ahsonkhan Jun 28, 2019

davidfowl commented Jun 29, 2019

Improve default handling in ReadOnlySequence.Slice #38431

Improve default handling in ReadOnlySequence.Slice #38431

Conversation

pgovind commented Jun 10, 2019 • edited Loading

ahsonkhan Jun 11, 2019 • 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

ahsonkhan commented Jun 11, 2019

pgovind commented Jun 12, 2019 • edited Loading

ahsonkhan left a 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

pgovind Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Jun 25, 2019 • 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

pgovind Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgovind commented Jun 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgovind commented Jun 26, 2019

ghost commented Jun 26, 2019

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Jun 29, 2019

pgovind commented Jun 10, 2019 •

edited

Loading

ahsonkhan Jun 11, 2019 •

edited

Loading

pgovind commented Jun 12, 2019 •

edited

Loading

pgovind Jun 24, 2019 •

edited

Loading

ahsonkhan Jun 25, 2019 •

edited

Loading

pgovind Jun 25, 2019 •

edited

Loading

pgovind commented Jun 26, 2019 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.