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.

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

Fix recent IndexOf regressions #80779

Merged
merged 2 commits into from
Jan 18, 2023
Merged

Conversation

MihaZupan
Copy link
Member

#78861 introduced new PackedIndexOf implementations for chars that have higher throughput for ASCII values.

It also regressed some scenarios as tracked by #80441. This PR fixes most of those regressions by:

  • dcc8763 which brings back the old codegen for IndexOf over non-chars (e.g. Span<byte>.IndexOf) by doing the type check earlier, before we call into CanUsePackedIndexOf.
  • 001b500 which avoids the overhead of checking whether a value can use the packed implementation where we wouldn't benefit anyway.

Example improvement for System.Net.Http because of dcc8763

Top method improvements (bytes):
    -22 (-2.46% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:ParseHeadersCore(System.Span`1[ubyte],System.Net.Http.HttpResponseMessage,bool):System.ValueTuple`2[bool,int]:this
    -5 (-3.45% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:<FillForHeadersAsync>g__TryFindEndOfLine|83_1(System.ReadOnlySpan`1[ubyte],byref):bool
    -5 (-1.99% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:ParseStatusLine(System.Net.Http.HttpResponseMessage):bool:this
    -5 (-1.82% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:TryReadNextChunkedLine(byref):bool:this

or on IndexOfAny for bytes:

Method Toolchain Mean Error Ratio
IndexOfAnyThreeValues before 2.985 ns 0.0166 ns 1.00
IndexOfAnyThreeValues main 4.085 ns 0.0573 ns 1.37
IndexOfAnyThreeValues pr 3.051 ns 0.0118 ns 1.02

@MihaZupan MihaZupan added this to the 8.0.0 milestone Jan 18, 2023
@MihaZupan MihaZupan self-assigned this Jan 18, 2023
@ghost
Copy link

ghost commented Jan 18, 2023

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

Issue Details

#78861 introduced new PackedIndexOf implementations for chars that have higher throughput for ASCII values.

It also regressed some scenarios as tracked by #80441. This PR fixes most of those regressions by:

  • dcc8763 which brings back the old codegen for IndexOf over non-chars (e.g. Span<byte>.IndexOf) by doing the type check earlier, before we call into CanUsePackedIndexOf.
  • 001b500 which avoids the overhead of checking whether a value can use the packed implementation where we wouldn't benefit anyway.

Example improvement for System.Net.Http because of dcc8763

Top method improvements (bytes):
    -22 (-2.46% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:ParseHeadersCore(System.Span`1[ubyte],System.Net.Http.HttpResponseMessage,bool):System.ValueTuple`2[bool,int]:this
    -5 (-3.45% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:<FillForHeadersAsync>g__TryFindEndOfLine|83_1(System.ReadOnlySpan`1[ubyte],byref):bool
    -5 (-1.99% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:ParseStatusLine(System.Net.Http.HttpResponseMessage):bool:this
    -5 (-1.82% of base) : System.Net.Http.dasm - System.Net.Http.HttpConnection:TryReadNextChunkedLine(byref):bool:this

or on IndexOfAny for bytes:

Method Toolchain Mean Error Ratio
IndexOfAnyThreeValues before 2.985 ns 0.0166 ns 1.00
IndexOfAnyThreeValues main 4.085 ns 0.0573 ns 1.37
IndexOfAnyThreeValues pr 3.051 ns 0.0118 ns 1.02
Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Memory

Milestone: 8.0.0

Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan MihaZupan merged commit c956f69 into dotnet:main Jan 18, 2023
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
* Improve IndexOf codegen for non-char types

* Call directly into NonPackedIndexOf where it makes sense
@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
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.

2 participants