Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use IndexOfAnyValues in System.Net.Http #78660

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Use IndexOfAnyValues in System.Net.Http #78660

merged 1 commit into from
Nov 22, 2022

Conversation

MihaZupan
Copy link
Member

Contributes to #78204

Example performance numbers for HttpRuleParser.IsToken taken from #78093's description:

Method Length Mean Error
IndexOfAnyValues_Char 1 2.683 ns 0.0108 ns
CurrentChar 1 1.655 ns 0.0046 ns
IndexOfAnyValues_Char 7 7.184 ns 0.0185 ns
CurrentChar 7 4.989 ns 0.1115 ns
IndexOfAnyValues_Char 8 2.402 ns 0.0025 ns
CurrentChar 8 4.787 ns 0.0155 ns
IndexOfAnyValues_Char 16 2.409 ns 0.0052 ns
CurrentChar 16 9.100 ns 0.0119 ns
IndexOfAnyValues_Char 32 3.306 ns 0.0078 ns
CurrentChar 32 19.486 ns 0.5098 ns
IndexOfAnyValues_Char 100000 6,004.741 ns 4.4187 ns
CurrentChar 100000 57,569.776 ns 49.4496 ns
IndexOfAnyValues_Byte 1 1.749 ns 0.0058 ns
CurrentByte 1 1.677 ns 0.0243 ns
IndexOfAnyValues_Byte 7 4.835 ns 0.0779 ns
CurrentByte 7 5.112 ns 0.0038 ns
IndexOfAnyValues_Byte 8 2.521 ns 0.0046 ns
CurrentByte 8 5.274 ns 0.0052 ns
IndexOfAnyValues_Byte 16 2.519 ns 0.0021 ns
CurrentByte 16 9.113 ns 0.0166 ns
IndexOfAnyValues_Byte 32 3.141 ns 0.0023 ns
CurrentByte 32 19.417 ns 1.0021 ns
IndexOfAnyValues_Byte 100000 4,762.442 ns 21.2032 ns
CurrentByte 100000 57,967.138 ns 97.4594 ns

@MihaZupan MihaZupan added this to the 8.0.0 milestone Nov 21, 2022
@MihaZupan MihaZupan requested a review from a team November 21, 2022 22:14
@ghost ghost assigned MihaZupan Nov 21, 2022
@ghost
Copy link

ghost commented Nov 21, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #78204

Example performance numbers for HttpRuleParser.IsToken taken from #78093's description:

Method Length Mean Error
IndexOfAnyValues_Char 1 2.683 ns 0.0108 ns
CurrentChar 1 1.655 ns 0.0046 ns
IndexOfAnyValues_Char 7 7.184 ns 0.0185 ns
CurrentChar 7 4.989 ns 0.1115 ns
IndexOfAnyValues_Char 8 2.402 ns 0.0025 ns
CurrentChar 8 4.787 ns 0.0155 ns
IndexOfAnyValues_Char 16 2.409 ns 0.0052 ns
CurrentChar 16 9.100 ns 0.0119 ns
IndexOfAnyValues_Char 32 3.306 ns 0.0078 ns
CurrentChar 32 19.486 ns 0.5098 ns
IndexOfAnyValues_Char 100000 6,004.741 ns 4.4187 ns
CurrentChar 100000 57,569.776 ns 49.4496 ns
IndexOfAnyValues_Byte 1 1.749 ns 0.0058 ns
CurrentByte 1 1.677 ns 0.0243 ns
IndexOfAnyValues_Byte 7 4.835 ns 0.0779 ns
CurrentByte 7 5.112 ns 0.0038 ns
IndexOfAnyValues_Byte 8 2.521 ns 0.0046 ns
CurrentByte 8 5.274 ns 0.0052 ns
IndexOfAnyValues_Byte 16 2.519 ns 0.0021 ns
CurrentByte 16 9.113 ns 0.0166 ns
IndexOfAnyValues_Byte 32 3.141 ns 0.0023 ns
CurrentByte 32 19.417 ns 1.0021 ns
IndexOfAnyValues_Byte 100000 4,762.442 ns 21.2032 ns
CurrentByte 100000 57,967.138 ns 97.4594 ns
Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http
Milestone: 8.0.0

@MihaZupan MihaZupan marked this pull request as ready for review November 21, 2022 22:14
stephentoub
stephentoub approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice.

@stephentoub stephentoub merged commit aa91451 into dotnet:main Nov 22, 2022
@MihaZupan
Copy link
Member Author

MihaZupan commented Nov 22, 2022 •
edited
Loading

I'm seeing failures on checked builds on ARM64 machines in #78678 that are likely related to changes to Encode5987 in this PR.

System.Net.Http.Tests.ContentDispositionHeaderValueTest.FileNameStar_NeedsEncoding_EncodedAndDecodedCorrectly [FAIL]
      Assert.Equal() Failure
                    ↓ (pos 4)
      Expected: FileÃName.bat
      Actual:   File?�Name.bat
                    ↑ (pos 4)
      Stack Trace:
        /_/src/libraries/System.Net.Http/tests/UnitTests/Headers/ContentDispositionHeaderValueTest.cs(211,0): at System.Net.Http.Tests.ContentDispositionHeaderValueTest.FileNameStar_NeedsEncoding_EncodedAndDecodedCorrectly()

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-78678-merge-3f1a8789e4f74bd6ac/System.Net.Http.Unit.Tests/3/console.0d41b14c.log?helixlogtype=result

https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-78678-merge-7f5df04042ea41ae83/System.Net.Http.Unit.Tests/3/console.17646da3.log?helixlogtype=result

cc: @EgorBo @tannergooding - this looks like different runtime behavior for Encode5987 on checked vs non-checked ARM64 builds (likely related to the new IndexOfAnyValues APIs added in #78093).

@stephentoub
Copy link
Member

It's showing up on other PRs as well.

@EgorBo
Copy link
Member

EgorBo commented Nov 22, 2022

I'll see if it's due to #78630 (via #78700)

jakobbotsch
jakobbotsch reviewed Nov 22, 2022
View reviewed changes
src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs
length = utf8.Length;
}

Encoding.ASCII.GetChars(utf8.Slice(0, length), builder.AppendSpan(length));
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be Encoding.ASCII?

Given that the failure is on alpine I wonder if it could be some locale issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't be affected by locale. The input here will always be these bytes (all in the [0, 127] range).

runtime/src/libraries/System.Net.Http/src/System/Net/Http/Headers/HeaderUtilities.cs

Lines 24 to 27 in 2ae7f5d

// attr-char = ALPHA / DIGIT / "!" / "#" / "$" / "&" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
// ; token except ( "*" / "'" / "%" )
private static readonly IndexOfAnyValues<byte> s_rfc5987AttrBytes =
IndexOfAnyValues.Create("!#$&+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~"u8);

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks. I also just noticed that there are non-alpine failures too.

Copy link
Member

Choose a reason for hiding this comment

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

https://runfo.azurewebsites.net/search/tests/?q=started%3A%7E7+definition%3Aruntime+name%3A%22FileNameStar_NeedsEncoding_EncodedAndDecodedCorrectly%22

Alpine and Ubuntu (both arm64)

@MihaZupan MihaZupan mentioned this pull request Nov 22, 2022
Merged
@EgorBo
Copy link
Member

EgorBo commented Nov 22, 2022

@MihaZupan no, it's not #78630 since revert still fails 🤔

@MihaZupan MihaZupan mentioned this pull request Nov 22, 2022
Closed
@MihaZupan
Copy link
Member Author

Doing some tests in #78709 to try and narrow it down

@MihaZupan MihaZupan mentioned this pull request Nov 22, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EgorBo EgorBo EgorBo left review comments

@jakobbotsch jakobbotsch jakobbotsch left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees

@MihaZupan MihaZupan

Labels
area-System.Net.Http
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@MihaZupan @stephentoub @EgorBo @jakobbotsch