Special case strings in System.Linq.Enumerable.OrderBy

[manofstick]

First in possibly a series of performance related pull requests to enhance performance of System.Linq. (Depending on how this is received...)

There changed originated from Cistern.Linq which is a complete overhaul of System.Linq architecture, but is probably a bit too much of a change for corefx to bear!

So anyway, this PR special cases string handling in OrderBy (when the default comparer is used - i.e. no extra parameter). I think this is a pretty common situation (although your telemetry might inform otherwise?). It increases performance because the comparer doesn't need to constantly get the current threads culture - which is consistent for the period of the sort, so an unnecessary overhead.

For strings like that of a person's name on a person object, this cuts about ~20% off the runtime, for a slow-down when there is only a single item to be sorted - I assume due to my additional check and replacement of the comparer, but could of just been an artifact of the particular run? Would be good for some other verification of alternative machines... Anyone?

Following benchmarking procedures, the following results were obtained:

(The additional performance test that is used here is in a PR here)

Slower	diff/base	Base Median (ns)	Diff Median (ns)	Modality
System.Linq.Tests.Perf_OrderBy.OrderByFirstName(NumberOfPeople: 1)	1.40	187.96	263.96
System.Linq.Tests.Perf_OrderBy.OrderByFullName(NumberOfPeople: 1)	1.22	352.27	430.88

Faster	base/diff	Base Median (ns)	Diff Median (ns)	Modality
System.Linq.Tests.Perf_OrderBy.OrderByFirstName(NumberOfPeople: 100)	1.26	52061.62	41456.28
System.Linq.Tests.Perf_OrderBy.OrderByFirstName(NumberOfPeople: 1000)	1.24	805309.06	647744.40
System.Linq.Tests.Perf_OrderBy.OrderByFirstName(NumberOfPeople: 10)	1.23	2538.67	2060.65
System.Linq.Tests.Perf_OrderBy.OrderByFullName(NumberOfPeople: 1000)	1.19	1020842.19	859375.00
System.Linq.Tests.Perf_OrderBy.OrderByFullName(NumberOfPeople: 100)	1.18	65284.81	55141.30
System.Linq.Tests.Perf_OrderBy.OrderByFullName(NumberOfPeople: 10)	1.07	3710.74	3481.37

[stephentoub]

I suspect we don't have any tests that validate culture behavior for string sorts / element ats / etc., including when an enumerable is enumerated multiple times with a different current culture each time, or when the culture changes between calls to MoveNext. Can you please add some?

[manofstick]

@stephentoub

Hmm. I think that there is an existing culture sorting issue that is unrelated to my PR on the Linux and Mac builds (that is I'm assuming that the Windows build does the correct thing, as the tests for me pass on windows on both core and framework - the tests were derived from the Sorting and String Comparison online docs).

I have conditionally excluded my change but get test failures such as this on the mac build:

    System.Linq.Tests.OrderByTests.CultureOrderByElementAt [FAIL]
      Assert.Equal() Failure
      Expected: Apple0
      Actual:   �ble0
      Stack Trace:
        /_/src/System.Linq/tests/OrderByTests.cs(523,0): at System.Linq.Tests.OrderByTests.CultureOrderByElementAt()
    System.Linq.Tests.OrderByTests.CultureOrderBy [FAIL]
      Assert.Equal() Failure
      Expected: String[] ["Apple0", "Apple1", "Apple2", "�ble0", "�ble1", ...]
      Actual:   String[] ["�ble0", "�ble1", "�ble2", "Apple0", "Apple1", ...]
      Stack Trace:
        /_/src/System.Linq/tests/OrderByTests.cs(469,0): at System.Linq.Tests.OrderByTests.CultureOrderBy()

The windows build failure seems unrelated also, especially given that my change is excluded...

[stephentoub]

I think that there is an existing culture sorting issue that is unrelated to my PR on the Linux and Mac builds (that is I'm assuming that the Windows build does the correct thing

.NET Core relies on the OS / underlying globalization support for culture-specific sorting tables. Currently on Windows that means using the data from Windows, and on Unix it means using ICU, and their sorting data can differ from each other. cc: @tarekgh

[tarekgh]

I think that there is an existing culture sorting issue that is unrelated to my PR on the Linux and Mac builds (that is I'm assuming that the Windows build does the correct thing

Something you may try and look if it will help, when calling OrderBy try to pass a comparer with ignore symbols. something like:

StringComparer comparer = StringComparer.Create(new CultureInfo("da-DK"), CompareOptions.IgnoreSymbols);
source.OrderBy(x => x, comparer)

[manofstick]

@tarekgh

Thanks for having a look, but the point of this PR is to handle the case where no comparer is passed to OrderBy , so to optimize the path of Comparer<string>.Default which will use the CurrentCulture, but as the thread doesn't change during the whole sort it only needs to access it once.

[manofstick]

I think we're all done...

[stephentoub]

Thanks. Other than a few nits, LGTM.

[maryamariyan]

Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

1. In your corefx repository clone, create patch by running git format-patch origin
2. In your runtime repository clone, apply the patch by running git apply --directory src/corefx <path to the patch created in step 1>

[maryamariyan]

Thanks @stephentoub for review.
Other than comments by @stephentoub, LGTM.

[manofstick]

Deloused those nits!

[stephentoub]

Thanks!