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

Address more issue numbers in the code #1888

Merged
merged 4 commits into from
Jan 18, 2020

Conversation

stephentoub
Copy link
Member

Replaced a bunch of unadorned issue numbers with links, so we know which repo the issue is for. In some cases the repo was named, but for consistency I still replaced it with a link.

In some cases, in particular where the issue had been closed saying effectively "we're not going to do anything here", I removed TODOs and the associated issue numbers.

In a few cases, the TODO and issue number was to say "we should do X when Y is fixed", so I did X since Y was fixed.

cc: @jaredpar, @danmosemsft

Replaced a bunch of unadorned issue numbers with links, so we know which repo the issue is for.  In some cases the repo was named, but for consistency I still replaced it with a link.

In some cases, in particular where the issue had been closed saying effectively "we're not going to do anything here", I removed TODOs and the associated issue numbers.

In a few cases, the TODO and issue number was to say "we should do X when Y is fixed", so I did X since Y was fixed.
@stephentoub stephentoub requested a review from a team January 17, 2020 23:00
@stephentoub
Copy link
Member Author

I hadn't planned on this being a perf-related PR, but as a happy accident, one of the issue numbers I was fixing up referred to a fix that could be made in LINQ's sorting implementation when a new Array.Sort overload was added. We ended up rejecting the proposal for that overload due to compatibility concerns, but the issue can be addressed similarly using the new span-based sorting methods we recently added.

The core sorting routine had been doing:

Array.Sort(keys, lo, hi - lo + 1, Comparer<int>.Create(CompareAnyKeys));

We have the CompareAnyKeys method, and we want to use it as the comparer for Array.Sort, but there's no overload that takes a Comparison<T>, so it's wrapped into a new IComparer<T>. There are a few issues with this. First, we're not only allocating a delegate for the CompareAnyKeys, we're also allocating an IComparer<T> to wrap it. Second, though, the Array.Sort<T> implementation actually works internally in terms of a Comparison<T>, so when it's given an IComparer<T>, it has to wrap its Compare method in a Comparison<T>. That means it ends up with a Comparison<T> wrapping the Compare method of an IComparer<T> that in turn invokes a Comparison<T>. Ugh. When we switch to using span:

new Span<int>(keys, lo, hi - lo + 1).Sort(CompareAnyKeys);

we pay for just one rather than two allocations, and we use the Comparison<T> directly rather than two additional levels of indirection.

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Linq;

public class Program
{
    static void Main(string[] args) => BenchmarkSwitcher.FromAssemblies(new[] { typeof(Program).Assembly }).Run(args);

    public Program()
    {
        var r = new Random(42);
        _array = new int[1_000];
        for (int i = 0; i < _array.Length; i++) _array[i] = r.Next();
    }

    private readonly int[] _array;

    [Benchmark]
    public void Sort()
    {
        foreach (int i in _array.OrderBy(i => i)) { }
    }
}
Method Toolchain Mean Error StdDev Ratio
Sort \master\corerun.exe 97.85 us 0.809 us 0.757 us 1.00
Sort \pr\corerun.exe 89.71 us 0.230 us 0.192 us 0.92

@jkotas jkotas merged commit ccf6aed into dotnet:master Jan 18, 2020
@stephentoub stephentoub deleted the moreissuecleanup branch January 22, 2020 01:41
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants