Skip to content

Fix recent IndexOf regressions - #80779

Merged
MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:packed-indexof-5
Jan 18, 2023
Merged

Fix recent IndexOf regressions#80779
MihaZupan merged 2 commits into
dotnet:mainfrom
MihaZupan:packed-indexof-5

Conversation

@MihaZupan

Copy link
Copy Markdown
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:

MethodToolchainMeanErrorRatio
IndexOfAnyThreeValuesbefore2.985 ns0.0166 ns1.00
IndexOfAnyThreeValuesmain4.085 ns0.0573 ns1.37
IndexOfAnyThreeValuespr3.051 ns0.0118 ns1.02

@MihaZupanMihaZupan added this to the 8.0.0 milestone Jan 18, 2023
@MihaZupanMihaZupan self-assigned this Jan 18, 2023
@ghost

Copy link
Copy Markdown

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:

MethodToolchainMeanErrorRatio
IndexOfAnyThreeValuesbefore2.985 ns0.0166 ns1.00
IndexOfAnyThreeValuesmain4.085 ns0.0573 ns1.37
IndexOfAnyThreeValuespr3.051 ns0.0118 ns1.02
Author:MihaZupan
Assignees:MihaZupan
Labels:

area-System.Memory

Milestone:8.0.0

@dakersnardakersnar left a comment

Copy link
Copy Markdown
Contributor

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:mainJan 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
@ghostghost locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for freeto 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

@MihaZupan@dakersnar