Skip to content

Improvements for SpanHelpers.IndexOf - #64872

Merged
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:revert-indexo
Feb 7, 2022
Merged

Improvements for SpanHelpers.IndexOf#64872
EgorBo merged 3 commits into
dotnet:mainfrom
EgorBo:revert-indexo

Conversation

@EgorBo

@EgorBoEgorBo commented Feb 6, 2022

Copy link
Copy Markdown
Member

Attempt to fix regressions caused by #63285 for large input: #64381

  1. use nint instead of int where possible to avoid sign extensions
  2. Slightly move "candidate found" block and assume we mostly find nothing (in theory, PGO will do the same)
  3. Hoist invariant searchSpaceMinusValueTailLength - Vector128<ushort>.Count expression - JIT only does it for natural loops so I had to do it by hands.
  4. Re-order cmpCh1 and cmpCh2 - it slightly improves pipelining (currently we emit a GPR instruction between two independent SIMD compare instructions)

Benchmarks: (Regressed ones and the ones from #63285)

MethodToolchainPatternMeanErrorStdDevRatio
Count\Core_Root_PR\corerun.exeSherlock44.90 us0.267 us0.237 us1.00
Count\Core_Root_base\corerun.exeSherlock51.14 us0.136 us0.120 us1.14
Count\Core_Root_PR\corerun.exeSherlock Holmes45.15 us0.281 us0.263 us1.00
Count\Core_Root_base\corerun.exeSherlock Holmes50.79 us0.089 us0.083 us1.12
Count\Core_Root_PR\corerun.exeSherlock\s+Holmes48.21 us0.182 us0.170 us1.00
Count\Core_Root_base\corerun.exeSherlock\s+Holmes53.07 us0.105 us0.093 us1.10
Count\Core_Root_PR\corerun.exeThe108.63 us0.157 us0.131 us1.00
Count\Core_Root_base\corerun.exeThe115.75 us0.115 us0.096 us1.07
Count\Core_Root_PR\corerun.exezqj34.84 us0.209 us0.195 us1.00
Count\Core_Root_base\corerun.exezqj39.76 us0.131 us0.123 us1.14
MethodToolchainMeanErrorStdDevRatio
IndexOf_Walking\Core_Root_PR\corerun.exe9.277 ns0.0282 ns0.0264 ns1.00
IndexOf_Walking\Core_Root_base\corerun.exe9.761 ns0.0105 ns0.0093 ns1.05
Contains_HtmlTag\Core_Root_PR\corerun.exe8.836 ns0.0257 ns0.0228 ns1.00
Contains_HtmlTag\Core_Root_base\corerun.exe9.044 ns0.0099 ns0.0087 ns1.02
IndexOf_Cmake\Core_Root_PR\corerun.exe17.686 ns0.0659 ns0.0617 ns1.00
IndexOf_Cmake\Core_Root_base\corerun.exe20.795 ns0.1066 ns0.0998 ns1.18

No changes for LastIndexOf for now (we also don't have reported regressions for it atm)

cc @stephentoub

@ghostghost assigned EgorBoFeb 6, 2022
@ghostghost added the area-System.Memory label Feb 6, 2022
@ghost

ghost commented Feb 6, 2022

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

Attempt to fix regressions caused by #63285 for large input: #64381

  1. use nint instead of int where possible to avoid sign extensions
  2. Slightly move "candidate found" block and assume we mostly find nothing
  3. Hoist invariant searchSpaceMinusValueTailLength - Vector128<ushort>.Count expression - JIT only does it for natural loops so I had to do it by hands.
  4. Re-order cmpCh1 and cmpCh2 - it slightly improves pipelining (currently we emit a GPR instruction between two SIMD compare)

Benchmarks: (Regressed ones and the ones from #63285)

MethodToolchainPatternMeanErrorStdDevRatio
Count\Core_Root_PR\corerun.exeSherlock44.90 us0.267 us0.237 us1.00
Count\Core_Root_base\corerun.exeSherlock51.14 us0.136 us0.120 us1.14
Count\Core_Root_PR\corerun.exeSherlock Holmes45.15 us0.281 us0.263 us1.00
Count\Core_Root_base\corerun.exeSherlock Holmes50.79 us0.089 us0.083 us1.12
Count\Core_Root_PR\corerun.exeSherlock\s+Holmes48.21 us0.182 us0.170 us1.00
Count\Core_Root_base\corerun.exeSherlock\s+Holmes53.07 us0.105 us0.093 us1.10
Count\Core_Root_PR\corerun.exeThe108.63 us0.157 us0.131 us1.00
Count\Core_Root_base\corerun.exeThe115.75 us0.115 us0.096 us1.07
Count\Core_Root_PR\corerun.exezqj34.84 us0.209 us0.195 us1.00
Count\Core_Root_base\corerun.exezqj39.76 us0.131 us0.123 us1.14
MethodToolchainMeanErrorStdDevRatio
IndexOf_Walking\Core_Root_PR\corerun.exe9.277 ns0.0282 ns0.0264 ns1.00
IndexOf_Walking\Core_Root_base\corerun.exe9.761 ns0.0105 ns0.0093 ns1.05
Contains_HtmlTag\Core_Root_PR\corerun.exe8.836 ns0.0257 ns0.0228 ns1.00
Contains_HtmlTag\Core_Root_base\corerun.exe9.044 ns0.0099 ns0.0087 ns1.02
IndexOf_Cmake\Core_Root_PR\corerun.exe17.686 ns0.0659 ns0.0617 ns1.00
IndexOf_Cmake\Core_Root_base\corerun.exe20.795 ns0.1066 ns0.0998 ns1.18

No changes for LastIndexOf for now (we also don't have reported regressions for it atm)

cc @stephentoub

Author:EgorBo
Assignees:EgorBo
Labels:

area-System.Memory

Milestone:-

@EgorBo

Copy link
Copy Markdown
MemberAuthor

NOTE: there is still a room for improvement, e.g. use the trick used in IndexOfAny - do the last iteration separately and it will allow us to remove a condition from the main loop.

@EgorBo
EgorBo merged commit bc9cdd1 into dotnet:mainFeb 7, 2022
@adamsitnikadamsitnik added the tenet-performance Performance related issue label Feb 7, 2022
@adamsitnikadamsitnik added this to the 7.0.0 milestone Feb 7, 2022
@ghostghost locked as resolved and limited conversation to collaborators Mar 9, 2022
@AndyAyersMS

Copy link
Copy Markdown
Member

@EgorBo the pattern here of branching back to LOOP_FOOTER confuses the JIT's loop recognition. I wonder if we'd be better off in the long run just duplicating the footer code?

(context: #87194 (comment))

@EgorBo
EgorBo deleted the revert-indexo branch July 25, 2023 00:45
@EgorBo

Copy link
Copy Markdown
MemberAuthor

@EgorBo the pattern here of branching back to LOOP_FOOTER confuses the JIT's loop recognition. I wonder if we'd be better off in the long run just duplicating the footer code?

(context: #87194 (comment))

That was sort of hand-written PGO and can be simplified now, although, NativeAOT might slightly regress

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Memorytenet-performancePerformance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@EgorBo@AndyAyersMS@stephentoub@adamsitnik