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

Improve IndexOfAny(byte,byte,byte) #40894

Merged
merged 3 commits into from
Oct 22, 2021

Conversation

benaadams
Copy link
Member

As per IndexOfAny(byte,byte) and IndexOfAny(char, ...)

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@adamsitnik adamsitnik added this to the 6.0.0 milestone Aug 17, 2020
@benaadams benaadams force-pushed the improved-IndexOfAny(byte-x3 branch from 647a01c to d5686ed Compare October 13, 2020 13:58
@benaadams benaadams force-pushed the improved-IndexOfAny(byte-x3 branch from d5686ed to c633e58 Compare November 14, 2020 12:50
@benaadams
Copy link
Member Author

Rebased

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

This looks good to me. @GrabYourPitchforks can you take a look at it please?

Base automatically changed from master to main March 1, 2021 09:06
@carlossanlop
Copy link
Member

carlossanlop commented Mar 15, 2021

@benaadams can you please share benchmarks for this change?

Also, is there an issue we can link to this PR?

@jeffhandley
Copy link
Member

@benaadams can you please share benchmarks for this change?

It would be great to see the deltas on this.

Also, is there an issue we can link to this PR?

Probably not; but that's fine.

@jeffhandley
Copy link
Member

@benaadams I think we are just waiting to see the benchmark results for this change, and we can presumably get this merged.

@benaadams
Copy link
Member Author

@jeffhandley can't currently build anything using .NET6.0 in VS dotnet/sdk#17461

@jeffhandley
Copy link
Member

@benaadams hopefully with the latest releases, this should be unblocked. Let us know if you're still having trouble though. Thanks!

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley
Copy link
Member

@GrabYourPitchforks This PR is assigned to you for follow-up/decision before the RC1 snap.

@adamsitnik adamsitnik modified the milestones: 6.0.0, 7.0.0 Aug 16, 2021
@adamsitnik
Copy link
Member

@jeffhandley @GrabYourPitchforks I've moved it to 7.0 as it's not crucial for the 6.0 release

@jeffhandley
Copy link
Member

@benaadams I've merged from main and verified that everything merged and builds cleanly. Could you run the affected perf tests and post the results to show the delta? Thanks!

@jeffhandley
Copy link
Member

I ran microbenchmarks on my desktop and saw the following results.

dotnet:main @ 221f1f5
BenchmarkDotNet=v0.13.1.1611-nightly, OS=Windows 10.0.22478
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.100-alpha.1.21518.14
  [Host]     : .NET 7.0.0 (7.0.21.48001), X64 RyuJIT
  Job-XZNBLJ : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog  Toolchain=CoreRun  
IterationTime=250.0000 ms  MaxIterationCount=20  MinIterationCount=15  
WarmupCount=1  
Method Size Mean Error StdDev Median Min Max Gen 0 Gen 1 Allocated
Clear 512 13.654 ns 0.2903 ns 0.2716 ns 13.667 ns 13.399 ns 14.235 ns - - -
Fill 512 11.703 ns 0.2575 ns 0.2755 ns 11.613 ns 11.436 ns 12.359 ns - - -
Reverse 512 127.007 ns 0.6267 ns 0.4893 ns 127.084 ns 126.494 ns 128.184 ns - - -
ToArray 512 27.515 ns 0.6097 ns 0.6776 ns 27.326 ns 26.489 ns 28.872 ns 0.0854 0.0001 536 B
SequenceEqual 512 15.738 ns 0.1936 ns 0.1616 ns 15.693 ns 15.567 ns 16.147 ns - - -
SequenceCompareTo 512 10.880 ns 0.1758 ns 0.1468 ns 10.823 ns 10.685 ns 11.132 ns - - -
StartsWith 512 9.315 ns 0.1188 ns 0.0927 ns 9.271 ns 9.189 ns 9.448 ns - - -
EndsWith 512 8.966 ns 0.1457 ns 0.1292 ns 8.920 ns 8.848 ns 9.208 ns - - -
IndexOfValue 512 9.794 ns 0.1532 ns 0.1433 ns 9.746 ns 9.661 ns 10.132 ns - - -
IndexOfAnyTwoValues 512 9.142 ns 0.0441 ns 0.0391 ns 9.128 ns 9.113 ns 9.227 ns - - -
IndexOfAnyThreeValues 512 12.792 ns 0.0746 ns 0.0582 ns 12.776 ns 12.731 ns 12.931 ns - - -
IndexOfAnyFourValues 512 675.663 ns 5.8364 ns 5.1738 ns 674.778 ns 669.693 ns 686.067 ns - - -
LastIndexOfValue 512 11.998 ns 1.7662 ns 2.0339 ns 10.686 ns 10.590 ns 15.505 ns - - -
LastIndexOfAnyValues 512 16.617 ns 1.5041 ns 1.7322 ns 17.351 ns 13.794 ns 18.549 ns - - -
BinarySearch 512 12.821 ns 0.2569 ns 0.2277 ns 12.758 ns 12.543 ns 13.334 ns - - -
GetPinnableReference 512 1.468 ns 0.0264 ns 0.0293 ns 1.466 ns 1.434 ns 1.534 ns - - -
benaadams:improved-IndexOfAny(byte-x3 @ eb85e70
BenchmarkDotNet=v0.13.1.1611-nightly, OS=Windows 10.0.22478
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.100-alpha.1.21518.14
  [Host]     : .NET 7.0.0 (7.0.21.48001), X64 RyuJIT
  Job-BMOQDB : .NET 7.0.0 (42.42.42.42424), X64 RyuJIT

PowerPlanMode=00000000-0000-0000-0000-000000000000  Arguments=/p:DebugType=portable,-bl:benchmarkdotnet.binlog  Toolchain=CoreRun  
IterationTime=250.0000 ms  MaxIterationCount=20  MinIterationCount=15  
WarmupCount=1  
Method Size Mean Error StdDev Median Min Max Gen 0 Gen 1 Allocated
Clear 512 13.636 ns 0.1417 ns 0.1326 ns 13.571 ns 13.500 ns 13.896 ns - - -
Fill 512 11.791 ns 0.1806 ns 0.1689 ns 11.809 ns 11.488 ns 12.024 ns - - -
Reverse 512 129.562 ns 2.6846 ns 3.0915 ns 128.808 ns 126.565 ns 136.745 ns - - -
ToArray 512 27.210 ns 0.5378 ns 0.5031 ns 27.165 ns 26.323 ns 28.034 ns 0.0854 0.0001 536 B
SequenceEqual 512 15.566 ns 0.1477 ns 0.1309 ns 15.508 ns 15.419 ns 15.807 ns - - -
SequenceCompareTo 512 10.865 ns 0.0739 ns 0.0617 ns 10.841 ns 10.781 ns 10.952 ns - - -
StartsWith 512 9.202 ns 0.1172 ns 0.0978 ns 9.163 ns 9.145 ns 9.433 ns - - -
EndsWith 512 9.019 ns 0.1752 ns 0.1553 ns 8.931 ns 8.854 ns 9.322 ns - - -
IndexOfValue 512 9.736 ns 0.2224 ns 0.2284 ns 9.723 ns 9.310 ns 10.164 ns - - -
IndexOfAnyTwoValues 512 9.257 ns 0.2136 ns 0.2194 ns 9.123 ns 9.085 ns 9.794 ns - - -
IndexOfAnyThreeValues 512 10.421 ns 0.1108 ns 0.0865 ns 10.399 ns 10.381 ns 10.695 ns - - -
IndexOfAnyFourValues 512 680.382 ns 6.4256 ns 5.6961 ns 682.220 ns 670.287 ns 686.652 ns - - -
LastIndexOfValue 512 15.032 ns 0.1523 ns 0.1425 ns 14.973 ns 14.902 ns 15.355 ns - - -
LastIndexOfAnyValues 512 20.806 ns 0.3905 ns 0.3835 ns 20.711 ns 20.393 ns 21.522 ns - - -
BinarySearch 512 12.795 ns 0.3130 ns 0.3605 ns 12.590 ns 12.441 ns 13.647 ns - - -
GetPinnableReference 512 1.469 ns 0.0428 ns 0.0492 ns 1.446 ns 1.425 ns 1.575 ns - - -

Comparison

summary:
better: 1, geomean: 1.229
worse: 2, geomean: 1.293
total diff: 3
Slower diff/base Base Median (ns) Diff Median (ns) Modality
LastIndexOfValue(Size: 512) 1.40 10.69 14.97 several?
LastIndexOfAnyValues(Size: 512) 1.19 17.35 20.71 several?
Faster base/diff Base Median (ns) Diff Median (ns) Modality
IndexOfAnyThreeValues(Size: 512) 1.23 12.78 10.40

Since LastIndexOfValue and LastIndexOfAnyValues were both unchanged and they both had a high standard error, I'm inclined to dismiss those results as noise.

@stephentoub or @jkotas -- what do you think? Are there any additional benchmark results you'd like to see?

@jeffhandley
Copy link
Member

Failing tests in System.Drawing are #60731 and #60751. Build failures ("The job running on agent NetCore1ESPool-Public 173 ran longer than the maximum time of 180 minutes") are unrelated.

@jeffhandley jeffhandley merged commit 30fb4c0 into dotnet:main Oct 22, 2021
@kunalspathak
Copy link
Member

kunalspathak commented Oct 26, 2021

Linux/x64 improvement - dotnet/perf-autofiling-issues#1983, dotnet/perf-autofiling-issues#2011

@kunalspathak
Copy link
Member

need to double check if we should also make equivalent changes in arm64
@echesakov

@jeffhandley
Copy link
Member

Great to see the results surface in the lab results. Thanks again for the PR, @benaadams.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants