Remove IsFastSort optimizations from String

[jkotas]

Fixes #26758

[jkotas]

Performance results:

The current microbenchmarks in performance repo are running the same tests on the same string, so they are helped by the cached state. For example, results for System.Tests.Perf_String.IndexOf:

Before:

Method	options	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
IndexOf	OrdinalIgnoreCase	131.131 us	0.8094 us	0.7175 us	130.867 us	130.497 us	132.775 us	-	-	-	-

After:

Method	options	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
IndexOf	OrdinalIgnoreCase	134.921 us	0.7286 us	0.6459 us	135.016 us	134.007 us	135.810 us	-	-	-	-

The scenario tested by the microbenchmarks is not realistic. It is unlikely that real workloads are going to be running the same IndexOf on two exact same string instances repeatedly. When the microbenchmark is modified to create a fresh instance of string using string.Copy, the version in this PR is faster.

Before:

Method	options	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
IndexOf	OrdinalIgnoreCase	139.426 us	1.4603 us	1.3660 us	138.840 us	137.604 us	142.475 us	1.6741	-	-	19.83 KB

After:

Method	options	Mean	Error	StdDev	Median	Min	Max	Gen 0	Gen 1	Gen 2	Allocated
IndexOf	OrdinalIgnoreCase	138.191 us	0.9291 us	0.8237 us	138.240 us	136.397 us	139.309 us	1.6304	-	-	19.83 KB

@adamsitnik Is it worth it to update the micro-benchmarks for IndexOf and similar operations to create string clones to be more realistic?

[adamsitnik]

Is it worth it to update the micro-benchmarks for IndexOf and similar operations to create string clones to be more realistic?

Yes. I am going to implement more realistic benchmarks dotnet/performance#885

[adamsitnik]

The Windows failures seems to be not related. I am going to re-run the tests

/azp run

[adamsitnik]

The Linux failures might have exposed differences between the string and ReadOnlySpan<char> text operations. I am going to try one thing to see if this is true.

Edit: I did not find any significant difference for StartsWith (see dotnet/corefx#41169)

[GrabYourPitchforks]

+1 to Tarek's comments re: improving test cases, but otherwise LGTM.

[jkotas]

@tarekgh The following test case is failing with my changes:

IsPrefix(compareInfo: CompareInfo - , source: "o\u0308", value: "o", options: None, expected: False)

We return true with my change because of o matches and it is in the ASCII set. We never look at \u0308. How is this supposed to work? Do we need to look at the following character in source, even after we find a match?

The same bug exists in Span version of StartsWith. We have test coverage hole for the Span version of StartsWith.

[tarekgh]

@jkotas

How is this supposed to work? Do we need to look at the following character in source, even after we find a match?

ö is not equal to o so the expected result is right (which is false). We'll need to look at the next character. we can look if the next character is NonSpacingMark character but still this may not be enough as we can encounter some characters with no sorting weight which I'll not be sure what would be the behavior (I can try that). Instead, I suggest if the next char is not Ascii, then just fallback to the slow path. If you want, I can try to think in a better way if we need to avoid going to the slow path.

[jkotas]

I suggest if the next char is not Ascii, then just fallback to the slow path.

I think that's fine. Thanks!

[jkotas]

I am fixing the test coverage hole for Span-based globalization methods in dotnet/corefx#41224

[adamsitnik]

@jkotas could you please wait with merging this PR until I finish dotnet/performance#892 ? I just want to make sure that this change is tracked properly in our performance reporting system. It should be a matter of hours from now

[adamsitnik]

@jkotas thanks for reviewing dotnet/performance#892 , I've merged it just right now. We run all benchmarks every 4 hours and it takes +-1h to run all Unix benchmarks (they are distributed to multiple machines). So 5h from now we can merge this PR. I hope it's not a problem!

[jkotas]

No problem. Thank you!