Implement ReadOnlySequence<T>.GetPosition(SequencePosition) #771

MarcoRossignoli · 2019-12-11T19:15:29Z

MarcoRossignoli · 2019-12-11T19:16:03Z

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

+            object? positionSequenceObject = position.GetObject();
+
+            // if sequence object is null we suppose start segment
+            if (!positionIsNotNull)


I used same semantic of Slice I don't know if it's ok, let me know

I haven't looked at the implementation too closely yet, but is there a benefit of the way GetOffset is implemented now instead of just doing the following:

GetOffset(SequencePosition position) => Slice(0, position).Length;

Are we doing different checks? Is it faster than doing all the checks that Slice/GetLength do that would be redundant here?

Probably creating an unnecessary/intermediary ReadOnlySequence would be too slow, but it might be worth measuring.

@ahsonkhan did some perf test and are similar(I need to understand from where that micro allocation comes)
No Slice

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

GetOffsetSingleSegment 73.78 ms 0.259 ms 0.230 ms - - - 138 B

GetOffsetMultiSegment 208.02 ms 0.669 ms 0.626 ms - - - 413 B

Slice

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

GetOffsetSingleSegment 87.31 ms 0.525 ms 0.491 ms - - - -

GetOffsetMultiSegment 211.03 ms 1.191 ms 1.056 ms - - - -

[MemoryDiagnoser] public class ReadOnlySequence { ReadOnlySequence<int> bufferSingleSegment; SequencePosition[] positionsSingleSegment; ReadOnlySequence<int> bufferMultiSegment; SequencePosition[] positionsMultiSegment; [GlobalSetup] public void Setup() { int n = 10_000_000; bufferSingleSegment = new ReadOnlySequence<int>(new int[n]); List<SequencePosition> positionsList = new List<SequencePosition>(); for (int i = 0; i <= bufferSingleSegment.Length; i++) { positionsList.Add(bufferSingleSegment.GetPosition(i)); } positionsSingleSegment = positionsList.ToArray(); positionsList.Clear(); var bufferSegment1 = new BufferSegment<int>(new int[n / 4]); BufferSegment<int> bufferSegment2 = bufferSegment1.Append(new int[n / 4]); BufferSegment<int> bufferSegment3 = bufferSegment2.Append(new int[n / 4]); BufferSegment<int> bufferSegment4 = bufferSegment3.Append(new int[n / 4]); bufferMultiSegment = new ReadOnlySequence<int>(bufferSegment1, 0, bufferSegment4, n / 4); for (int i = 0; i <= bufferMultiSegment.Length; i++) { positionsList.Add(bufferMultiSegment.GetPosition(i)); } positionsMultiSegment = positionsList.ToArray(); positionsList.Clear(); } [Benchmark] public void GetOffsetSingleSegment() { for (int i = 0; i <= bufferSingleSegment.Length; i++) { bufferSingleSegment.GetOffset(positionsSingleSegment[i]); } } [Benchmark] public void GetOffsetMultiSegment() { for (int i = 0; i <= bufferMultiSegment.Length; i++) { bufferMultiSegment.GetOffset(positionsMultiSegment[i]); } } }

BTW the most important thing is that slice doesn't work as expected on boundary and test doesn't pass for
GetOffset_MultiSegment_PositionOutOfRange_SegmentNotFound
GetOffset_MultiSegment_BoundaryConditions
GetOffset_MultiSegment_SequencePositionSegment
GetOffset_MultiSegment_PositionOutOfRange_SegmentNotFound
GetOffset_MultiSegment_BoundaryConditions
GetOffset_MultiSegment_SequencePositionSegment

What do you think?

Interesting. I am surprised by the "no slice" method showing allocations as well. Could you measure just a single call to GetOffset rather than doing it in the loop (and let BDN deal with running it many times)?

I will take a closer look tomorrow at the tests failing if we chose to go with the simpler Slice approach. Maybe we make Slice more resilient to those edge cases.

Letting BDN to profile e chose right iteration count

[Benchmark] public void GetOffsetSingleSegment() { bufferSingleSegment.GetOffset(positionsSingleSegment[9_000_000]); } [Benchmark] public void GetOffsetMultiSegment() { bufferMultiSegment.GetOffset(positionsMultiSegment[9_000_000]); }

No slice

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

GetOffsetSingleSegment 6.311 ns 0.0351 ns 0.0329 ns - - - -

GetOffsetMultiSegment 19.354 ns 0.0394 ns 0.0329 ns - - - -

Slice

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated

GetOffsetSingleSegment 15.61 ns 0.057 ns 0.053 ns - - - -

GetOffsetMultiSegment 25.62 ns 0.074 ns 0.065 ns - - - -

MarcoRossignoli · 2019-12-11T19:17:03Z

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

+        public long GetOffset(SequencePosition position)
+        {
+            bool positionIsNotNull = position.GetObject() != null;
+            BoundsCheck(position, positionIsNotNull);


I did also a version with check inside method, but I've opted for clear code/code reuse, I don't think this is a "hot path" api.

src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs

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

MarcoRossignoli · 2019-12-12T09:19:09Z

/azp

MarcoRossignoli · 2019-12-12T09:20:35Z

/azp runtime-libraries

azure-pipelines · 2019-12-12T09:20:40Z

Command 'runtime-libraries' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run. Example: "run" or "run pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

MarcoRossignoli · 2019-12-12T09:21:10Z

/azp list

azure-pipelines · 2019-12-12T09:21:16Z

Commenter does not have sufficient privileges for PR 771 in repo dotnet/runtime

MarcoRossignoli · 2019-12-12T09:22:03Z

/azp run runtime-libraries

azure-pipelines · 2019-12-12T09:22:08Z

Commenter does not have sufficient privileges for PR 771 in repo dotnet/runtime

MarcoRossignoli · 2019-12-12T09:24:37Z

@terrajobst I've accepted the invite to external-ci-access but I get error.

cc: @safern @ViktorHofer

ViktorHofer · 2019-12-12T10:08:31Z

/azp run runtime-libraries

azure-pipelines · 2019-12-12T10:08:45Z

Azure Pipelines successfully started running 1 pipeline(s).

terrajobst · 2019-12-12T20:27:47Z

@terrajobst I've accepted the invite to external-ci-access but I get error.

Sorry about that. I've asked @mmitche to take a look.

MarcoRossignoli · 2019-12-12T20:38:09Z

No worries thanks a lot for quick response!

MarcoRossignoli · 2020-01-10T19:10:05Z

ping @ahsonkhan @JeremyKuhne

stephentoub · 2020-01-15T14:41:29Z

cc: @pakrym, @ahsonkhan

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

src/libraries/System.Memory/src/System/ThrowHelper.cs

src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs

ahsonkhan · 2020-01-23T06:25:49Z

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

@@ -528,6 +528,56 @@ public SequencePosition GetPosition(long offset)
            return Seek(offset);
        }

+        /// <summary>
+        /// Translate opaque <see cref="System.SequencePosition"/> value to numerical position offset


The way this is written, it gives the impression that any arbitrary SequencePosition can be passed in and is valid. Also, using translate seems strange to me.

We should try to be consistent with existing docs:
https://docs.microsoft.com/en-us/dotnet/api/system.buffers.readonlysequence-1.getposition?view=netcore-3.1

How about:

Suggested change

/// Translate opaque <see cref="System.SequencePosition"/> value to numerical position offset

/// Returns the offset of a <paramref name="position" /> within this sequence from the start.

We should also add a remarks section stating that callers should only pass in a SequencePosition that belong to this ReadOnlySequence. Otherwise, the behavior is undefined.

The other thing that might be worth noting in the remarks (and testing for) is that the behavior is "exclusive", not "inclusive".

Let's say we have one of these sequences:
[50] -> [0]
[50] -> [50]

What does calling GetOffset(new SequencePosition(second segment, 0)) return? 50, correct? And then if I slice the ReadOnlySequence from 0 to that offset (i.e. 50), is the returned ReadOnlySequence, a single segment?

Something like:

ReadOnlySequence<byte> slice = ros.GetOffset(new SequencePosition(segment2, 0)); Assert.True(slice.IsSingleSegment); slice = ros.GetOffset(new SequencePosition(segment1, 50)); // This would be the exclusive case. Assert.True(slice.IsSingleSegment); slice = ros.GetOffset(new SequencePosition(segment1, 51)); // This will throw, correct?

@ahsonkhan

slice = ros.Slice(new SequencePosition(segment1, 50)); // This would be the exclusive case. Assert.True(slice.IsSingleSegment);

this is not a single segment...the returned sequence is a multi segment starting from 50th item.

ReadOnlySequence<byte> slice = ros.Slice(new SequencePosition(segment2, 0)); Assert.True(slice.IsSingleSegment); [50] -> [0] returned new ROS startObj segment2 endObj segment2 so IsSingleSegment = true slice = ros.Slice(new SequencePosition(segment1, 50)); // This would be the exclusive case. Assert.True(slice.IsSingleSegment); [50] -> [0] returned new ROS startObj segment1 endObj segment2 so IsSingleSegment = false slice = ros.Slice(new SequencePosition(segment1, 51)); // This will throw, correct? <-- Yes BoundsCheck()

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

Co-Authored-By: Ahson Khan <ahkha@microsoft.com>

… condition.

MarcoRossignoli · 2020-02-07T16:11:39Z

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

+                // For single segment position bound check is implicit.
+                Debug.Assert(positionSequenceObject != null);
+
+                if (((ReadOnlySequenceSegment<T>)positionSequenceObject).Memory.Length - positionIndex < 0)


@ahsonkhan I don't know if this is ok, default BoundCheck method doesn't check for SequencePosition validity, but only if offset specified is valid against current ROSequence.
It's implicit for single segment because the object is the same(if sequence position is pertinent) and only indexes are checked.

ahsonkhan

Some nits.

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

src/libraries/System.Memory/ref/System.Memory.cs

src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs

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

adamsitnik · 2020-08-14T14:03:30Z

/azp run

azure-pipelines · 2020-08-14T14:03:35Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

adamsitnik · 2020-08-17T07:38:00Z

/azp run runtime-libraries

azure-pipelines · 2020-08-17T07:38:05Z

No pipelines are associated with this pull request.

Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

adamsitnik

@MarcoRossignoli thank you for adding good tests and comments which made the review easy for me.

If CI passes today, it's going to be a part of 5.0.

Fixes dotnet#771

Dotnet-GitSync-Bot added the area-System.Memory label Dec 11, 2019

MarcoRossignoli commented Dec 11, 2019

View reviewed changes

src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs Show resolved Hide resolved

akashkc reviewed Dec 11, 2019

View reviewed changes

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

carlossanlop reviewed Jan 21, 2020

View reviewed changes

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

carlossanlop added the new-api-needs-documentation label Jan 21, 2020

ahsonkhan reviewed Jan 23, 2020

View reviewed changes

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

MarcoRossignoli and others added 7 commits February 7, 2020 14:39

add GetOffset(long) api

fb2288b

address PR feedback

2a2329e

simplify code

c5d0aae

update documentation

d89937b

Apply suggestions from code review

e287d39

Co-Authored-By: Ahson Khan <ahkha@microsoft.com>

revert change, annotate null ref

67f0d43

add check for invalid passed SequencePosition, add tests for boundary…

b449ed0

… condition.

add enumerate and slice tests.

22cdb59

MarcoRossignoli commented Feb 7, 2020

View reviewed changes

Fix method documentation

6e19c26

ahsonkhan reviewed Feb 12, 2020

View reviewed changes

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

src/libraries/System.Memory/ref/System.Memory.cs Outdated Show resolved Hide resolved

ahsonkhan reviewed Feb 12, 2020

View reviewed changes

src/libraries/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs Outdated Show resolved Hide resolved

MarcoRossignoli added 2 commits February 12, 2020 13:04

address PR feedback

658b9e6

fix codege

a68dbb2

MarcoRossignoli requested a review from ahsonkhan February 12, 2020 12:08

This was referenced Feb 12, 2020

System.Net.NetworkInformation.Functional.Tests failing CI and PR #32179

Closed

Test failure: System.Net.NetworkInformation.Tests.NetworkInterfaceBasicTest/BasicTest_AccessInstanceProperties_NoExceptions_Linux #18090

Closed

carlossanlop reviewed Feb 18, 2020

View reviewed changes

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

adamsitnik added this to the 5.0.0 milestone Aug 14, 2020

adamsitnik closed this Aug 17, 2020

adamsitnik reopened this Aug 17, 2020

improve xml comment

00241b1

Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

adamsitnik approved these changes Aug 17, 2020

View reviewed changes

adamsitnik merged commit 2d93df6 into dotnet:master Aug 17, 2020

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this pull request Mar 25, 2021

Add workaround for DuplicatePublishOutput check (dotnet#773)

75feafe

Fixes dotnet#771

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ReadOnlySequence<T>.GetPosition(SequencePosition) #771

Implement ReadOnlySequence<T>.GetPosition(SequencePosition) #771

MarcoRossignoli commented Dec 11, 2019 •

edited by carlossanlop

Loading

MarcoRossignoli Dec 11, 2019

ahsonkhan Jan 23, 2020 •

edited

Loading

MarcoRossignoli Feb 11, 2020 •

edited

Loading

ahsonkhan Feb 12, 2020

MarcoRossignoli Feb 12, 2020 •

edited

Loading

MarcoRossignoli Dec 11, 2019

MarcoRossignoli commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019 •

edited

Loading

ViktorHofer commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

terrajobst commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

MarcoRossignoli commented Jan 10, 2020

stephentoub commented Jan 15, 2020

ahsonkhan Jan 23, 2020

ahsonkhan Jan 23, 2020 •

edited

Loading

MarcoRossignoli Feb 7, 2020 •

edited

Loading

MarcoRossignoli Feb 7, 2020 •

edited

Loading

ahsonkhan left a comment

adamsitnik commented Aug 14, 2020

azure-pipelines bot commented Aug 14, 2020

adamsitnik commented Aug 17, 2020

azure-pipelines bot commented Aug 17, 2020

adamsitnik left a comment

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
GetOffsetSingleSegment	73.78 ms	0.259 ms	0.230 ms	-	-	-	138 B
GetOffsetMultiSegment	208.02 ms	0.669 ms	0.626 ms	-	-	-	413 B

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
GetOffsetSingleSegment	87.31 ms	0.525 ms	0.491 ms	-	-	-	-
GetOffsetMultiSegment	211.03 ms	1.191 ms	1.056 ms	-	-	-	-

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
GetOffsetSingleSegment	6.311 ns	0.0351 ns	0.0329 ns	-	-	-	-
GetOffsetMultiSegment	19.354 ns	0.0394 ns	0.0329 ns	-	-	-	-

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
GetOffsetSingleSegment	15.61 ns	0.057 ns	0.053 ns	-	-	-	-
GetOffsetMultiSegment	25.62 ns	0.074 ns	0.065 ns	-	-	-	-

	/// Translate opaque <see cref="System.SequencePosition"/> value to numerical position offset
	/// Returns the offset of a <paramref name="position" /> within this sequence from the start.

Implement ReadOnlySequence<T>.GetPosition(SequencePosition) #771

Implement ReadOnlySequence<T>.GetPosition(SequencePosition) #771

Conversation

MarcoRossignoli commented Dec 11, 2019 • edited by carlossanlop Loading

MarcoRossignoli Dec 11, 2019

Choose a reason for hiding this comment

ahsonkhan Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Feb 11, 2020 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Feb 12, 2020

Choose a reason for hiding this comment

MarcoRossignoli Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Dec 11, 2019

Choose a reason for hiding this comment

MarcoRossignoli commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019 • edited Loading

ViktorHofer commented Dec 12, 2019

azure-pipelines bot commented Dec 12, 2019

terrajobst commented Dec 12, 2019

MarcoRossignoli commented Dec 12, 2019

MarcoRossignoli commented Jan 10, 2020

stephentoub commented Jan 15, 2020

ahsonkhan Jan 23, 2020

Choose a reason for hiding this comment

ahsonkhan Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

adamsitnik commented Aug 14, 2020

azure-pipelines bot commented Aug 14, 2020

adamsitnik commented Aug 17, 2020

azure-pipelines bot commented Aug 17, 2020

adamsitnik left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented Dec 11, 2019 •

edited by carlossanlop

Loading

ahsonkhan Jan 23, 2020 •

edited

Loading

MarcoRossignoli Feb 11, 2020 •

edited

Loading

MarcoRossignoli Feb 12, 2020 •

edited

Loading

MarcoRossignoli commented Dec 12, 2019 •

edited

Loading

ahsonkhan Jan 23, 2020 •

edited

Loading

MarcoRossignoli Feb 7, 2020 •

edited

Loading

MarcoRossignoli Feb 7, 2020 •

edited

Loading