-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Use SpanHelpers.SequenceCompareTo for String.CompareOrdinal #111586
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
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
|
You should test perf for longer strings as well. CompareOrdinalHelper implementation is specifically tuned for string alignment that is only going to show up on longer strings https://github.com/dotnet/runtime/pull/111586/files#diff-79247fffe0432073a5ddd0de692f4beaf015c0c483dd311a921393d05568a3e4L119-L122 |
Well there is regression at ~40 length, where SIMD should start to kick in. |
|
@EgorBot -intel -amd using System;
using System.Collections.Generic;
using System.Linq;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
namespace BenchmarkGround
{
public class Bench
{
public IEnumerable<object[]> Args =>
[
["Foo", "FooBar"],
["Foo", "Bar"],
["A quick brown fox jumps over a lazy dog", "A quick brown fox jumps over a lazy hog"],
[new string([.. Enumerable.Repeat('a', 1000), '1']), new string([.. Enumerable.Repeat('a', 1000), '2'])],
];
[Benchmark]
[ArgumentsSource(nameof(Args))]
public int Compare(string left, string right) => string.Compare(left, right, StringComparison.Ordinal);
[Benchmark]
[ArgumentsSource(nameof(Args))]
public int CompareOridinal(string left, string right) => string.CompareOrdinal(left, right);
}
} |
|
@huoyaoyuan I was wondering if I should add that ability to define simple benchmarks without classes too 🤔 |
I was making more mistakes for example missing Actually I'm getting help from bot just because benchmark in Windows Terminal over RDP looks unstable. |
|
The benchmark shows small regression on Cascade Lake/small length. It's similar to what I see on Raptor Lake. What's the preference for the action here? |
Since it's Intel only it is most likely JCC Erratum |
src/libraries/System.Private.CoreLib/src/System/String.Comparison.cs
Outdated
Show resolved
Hide resolved
|
The change looks good to me. The PR comments apply to both the new code as well as the old code without these changes, so I think they can be addressed afterwards. @jkotas thoughts? |
|
@EgorBot -amd -arm --envvars DOTNET_ReadyToRun:0 DOTNET_TieredCompilation:0 |
|
@EgorBot -arm |
|
@EgorBot -amd -arm --envvars DOTNET_ReadyToRun:0 DOTNET_TieredCompilation:0 |
|
@EgorBot -arm |
Helps reusing centralized code under
SpanHelpers. There shouldn't be much change for performance, just ensuring no regression.Benchmark: