Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add ReadOnlySequence<T>.FirstSpan#36194

Merged
maryamariyan merged 4 commits intodotnet:masterfrom
maryamariyan:first-span
Mar 29, 2019
Merged

Add ReadOnlySequence<T>.FirstSpan#36194
maryamariyan merged 4 commits intodotnet:masterfrom
maryamariyan:first-span

Conversation

@maryamariyan
Copy link
Copy Markdown

@maryamariyan maryamariyan commented Mar 20, 2019

Fixes: #33029

  • Expose API
  • Add unit tests
  • Add perf tests

cc: @ahsonkhan

@maryamariyan maryamariyan self-assigned this Mar 20, 2019
Comment thread src/System.Memory/src/System/Buffers/ReadOnlySequence.cs Outdated
@maryamariyan maryamariyan added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2019
@maryamariyan maryamariyan changed the title Add ReadOnlySequence<T>.FirstSpan [WIP] Add ReadOnlySequence<T>.FirstSpan Mar 21, 2019
@ahsonkhan ahsonkhan added this to the 3.0 milestone Mar 21, 2019
@ahsonkhan
Copy link
Copy Markdown

@maryamariyan, consider making this a draft PR while in progress. This way, the WIP/No merge label become unnecessary. Something to consider for the future.

See https://github.blog/2019-02-14-introducing-draft-pull-requests/

@maryamariyan maryamariyan changed the title [WIP] Add ReadOnlySequence<T>.FirstSpan Add ReadOnlySequence<T>.FirstSpan Mar 25, 2019
@maryamariyan maryamariyan removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 25, 2019
Comment thread src/System.Memory/src/System/Buffers/ReadOnlySequence.Helpers.cs
Comment thread src/System.Memory/src/System/Buffers/ReadOnlySequence.Helpers.cs Outdated
@maryamariyan
Copy link
Copy Markdown
Author

maryamariyan commented Mar 28, 2019

I added benchmarks for FirstSpan in the new performance repo. (dotnet/performance#410 (comment))
Below shows comparison between using First.Span vs the new FirstSpan API

|                  Method |     Mean |     Error |    StdDev |   Median |      Min |      Max | 
|------------------------ |---------:|----------:|----------:|---------:|---------:|---------:|
| First.Span for Array    | 28.58 ns | 0.3199 ns | 0.2835 ns | 28.52 ns | 28.22 ns | 29.33 ns |
| FirstSpan for Array     | 20.11 ns | 0.7130 ns | 0.8211 ns | 19.68 ns | 19.29 ns | 22.48 ns |

| First.Span for Memory   | 29.21 ns | 0.1973 ns | 0.1648 ns | 29.20 ns | 28.93 ns | 29.49 ns |
| FirstSpan for Memory    | 21.75 ns | 0.4870 ns | 0.4317 ns | 21.62 ns | 21.30 ns | 22.86 ns |

| First.Span for 1 Segment  | 26.07 ns | 0.5792 ns | 0.5418 ns | 25.92 ns | 25.59 ns | 27.39 ns |
| FirstSpan for 1 Segment   | 25.38 ns | 0.8425 ns | 0.9015 ns | 24.97 ns | 24.54 ns | 27.50 ns |

| First.Span for 10 Segments | 26.05 ns | 0.9496 ns | 1.0161 ns | 25.73 ns | 25.13 ns | 28.96 ns |
| FirstSpan for 10 Segments  | 23.69 ns | 0.1279 ns | 0.0999 ns | 23.67 ns | 23.57 ns | 23.86 ns |

|First.Span for MemoryManager| 40.65 ns | 0.9715 ns | 0.9542 ns | 40.39 ns | 39.63 ns | 42.59 ns |
| FirstSpan for MemoryManager| 28.41 ns | 0.7400 ns | 0.6922 ns | 28.16 ns | 27.85 ns | 30.22 ns |

| First.Span for String | 26.21 ns | 0.1975 ns | 0.1542 ns | 26.20 ns | 25.90 ns | 26.44 ns |
| FirstSpan for String  | 23.59 ns | 1.1345 ns | 1.3065 ns | 22.98 ns | 22.52 ns | 26.28 ns |

Copy link
Copy Markdown

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, LGTM

Comment thread src/System.Memory/src/System/Buffers/ReadOnlySequence.Helpers.cs Outdated
@ahsonkhan
Copy link
Copy Markdown

I added benchmarks for FirstSpan in the new performance repo

How about for string and MemoryManager cases?

@maryamariyan
Copy link
Copy Markdown
Author

maryamariyan commented Mar 28, 2019

I just updated my comment to also include numbers for string and MemoryManager

Comment thread src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.Common.cs Outdated
@maryamariyan
Copy link
Copy Markdown
Author

Linux failure related to Fedora 27 queues leaving Helix.

@maryamariyan maryamariyan merged commit 8292e45 into dotnet:master Mar 29, 2019
@maryamariyan maryamariyan deleted the first-span branch March 29, 2019 18:19
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Add ReadOnlySequence<T>.FirstSpan

Fixes: dotnet/corefx#33029


Commit migrated from dotnet/corefx@8292e45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants