Skip to content

Improve SpanHelpers.IndexOfAny(byte,byte)#40747

Merged
kunalspathak merged 3 commits intodotnet:masterfrom
benaadams:Improve-SpanHelpers.IndexOfAny(byte,byte)-
Aug 14, 2020
Merged

Improve SpanHelpers.IndexOfAny(byte,byte)#40747
kunalspathak merged 3 commits intodotnet:masterfrom
benaadams:Improve-SpanHelpers.IndexOfAny(byte,byte)-

Conversation

@benaadams
Copy link
Copy Markdown
Member

@benaadams benaadams commented Aug 13, 2020 •
edited
Loading

The better approached used in #40729

| Faster                                                   | base/diff | Base Median (ns) | Diff Median (ns) | Modality|
| -------------------------------------------------------- | ---------:| ----------------:| ----------------:| --------:|
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 16)   |      1.45 |             5.02 |             3.45 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 24)   |      1.52 |             5.93 |             3.91 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 32)   |      1.44 |             6.09 |             4.23 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 48)   |      1.69 |             5.82 |             3.45 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 64)   |      1.40 |             5.82 |             4.17 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 128)  |      1.29 |             6.36 |             4.94 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 512)  |      1.43 |            10.75 |             7.54 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 256)  |      1.34 |             7.58 |             5.67 |         |
| System.Memory.Span<Byte>.IndexOfAnyTwoValues(Size: 1024) |      1.11 |            12.99 |            11.69 |         |

Will effect header parsing in Kestrel

length = span.IndexOfAny(ByteCR, ByteLF)

/cc @adamsitnik @danmosemsft

@benaadams benaadams force-pushed the Improve-SpanHelpers.IndexOfAny(byte,byte)- branch from f7652c5 to 6e7f28e Compare August 13, 2020 03:41
@benaadams benaadams marked this pull request as ready for review August 13, 2020 04:33
gfoidl
gfoidl reviewed Aug 13, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Typo.

Comment thread src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs Outdated
@benaadams @gfoidl
Update src/libraries/System.Private.CoreLib/src/System/SpanHelpers.By…
779c913 
…te.cs

Co-authored-by: Günther Foidl <gue@korporal.at>
danmoseley
danmoseley reviewed Aug 14, 2020
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs Outdated
@benaadams @danmoseley
Update src/libraries/System.Private.CoreLib/src/System/SpanHelpers.By…
3a0465d 
…te.cs

Co-authored-by: Dan Moseley <danmose@microsoft.com>
@danmoseley
Copy link
Copy Markdown
Member

danmoseley commented Aug 14, 2020

@benaadams @sebastienros is there a way to quantify how much it would help Kestrel? Would help prioritize review.

@benaadams
Copy link
Copy Markdown
Member Author

benaadams commented Aug 14, 2020

Top one of the SpanHelpers for PlainText Platform on Citrine on Linux

image

Not sure how it changes it though :)

@adamsitnik adamsitnik added the tenet-performance Performance related issue label Aug 14, 2020
@adamsitnik adamsitnik added this to the 5.0.0 milestone Aug 14, 2020
kunalspathak
kunalspathak reviewed Aug 14, 2020
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs
lengthToExamine = ((nuint)(uint)length - offset);
goto SequentialScan;
}
if (!TryFindFirstMatchedLane(mask, matches, ref matchedLane))
Copy link
Copy Markdown
Contributor

@kunalspathak kunalspathak Aug 14, 2020

Choose a reason for hiding this comment

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

Thanks for doing this for ARM as well.

kunalspathak
kunalspathak approved these changes Aug 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing this.

jeffhandley
jeffhandley approved these changes Aug 14, 2020
View reviewed changes
Comment thread src/libraries/System.Private.CoreLib/src/System/SpanHelpers.Byte.cs
Comment on lines +609 to +613
// >= Sse2 intrinsics are supported, and length is enough to use them so use that path.
// We jump forward to the intrinsics at the end of the method so a naive branch predict
// will choose the non-intrinsic path so short lengths which don't gain anything aren't
// overly disadvantaged by having to jump over a lot of code. Whereas the longer lengths
// more than make this back from the intrinsics.
Copy link
Copy Markdown
Member

@jeffhandley jeffhandley Aug 14, 2020

Choose a reason for hiding this comment

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

Beautiful comment; thank you.

@kunalspathak kunalspathak merged commit b932a51 into dotnet:master Aug 14, 2020
@benaadams benaadams deleted the Improve-SpanHelpers.IndexOfAny(byte,byte)- branch August 14, 2020 17:45
@danmoseley
Copy link
Copy Markdown
Member

danmoseley commented Aug 14, 2020

@sebastienros do you expect this to materially improve Plaintext, given data above?

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Aug 14, 2020

We can hope it doesn't make it slower! Ideally any perf related work should be measured before merged, even more that close to rc. We'll have to look at the charts once the build is available. We can also do a custom with the next build containing this change. I'll follow the CI.

@jeffhandley
Copy link
Copy Markdown
Member

jeffhandley commented Aug 14, 2020

I recommended to @kunalspathak that we go ahead and merge, but good point, @sebastienros. Thanks for checking on it.

@adamsitnik
Copy link
Copy Markdown
Member

adamsitnik commented Aug 17, 2020

@benaadams thank you for another great improvement!

@kunalspathak big thanks for reviewing and merging the PR!

:shipit:

@ahsonkhan ahsonkhan mentioned this pull request Sep 4, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@danmoseley danmoseley danmoseley left review comments

@jeffhandley jeffhandley jeffhandley approved these changes

+2 more reviewers

@gfoidl gfoidl gfoidl left review comments

@kunalspathak kunalspathak kunalspathak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Memory tenet-performance Performance related issue

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

8 participants

@benaadams @danmoseley @sebastienros @jeffhandley @adamsitnik @gfoidl @kunalspathak @Dotnet-GitSync-Bot