Change ArraySortHelper to use Comparison<T>

[mikedn]

The Array/List.Sort overloads that take a Comparison<T> have worse performance than the ones that take a IComparer<T>. That's because sorting is implemented around IComparer<T> and a Comparison<T> needs to be wrapped in a comparer object to be used.

At the same time, interface calls are slower than delegate calls so the existing implementation doesn't offer the best performance even when the IComparer<T> based overloads are used.

By changing the implementation to use Comparison<T> we avoid interface calls in both cases.

When IComparer<T> overloads are used a Comparison<T> delegate is created from IComparer<T>.Compare, that's an extra object allocation but sorting is faster and we avoid having two separate sorting implementations.

Port of dotnet/coreclr#8504

[dnfclas]

Hi @mikedn, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[MichalStrehovsky]

@mikedn I'll take a look at the Generics test failing tomorrow. This is unlikely to be a problem in your change. Shared generics are not completely done yet and this is either uncovering a bug or hitting a missing feature in shared generics.

[mikedn]

Presumably this is caused by IntrospectiveSort(keys, index, length, comparer.Compare); where a delegate is bound to an interface method. Otherwise there's little else that's new, methods that used delegate existed before.

And the compilation exception is:

  Unhandled Exception: System.NotImplementedException: CORINFO_HELP_VIRTUAL_FUNC_PTR
     at Internal.JitInterface.CorInfoImpl.getHelperFtn(CorInfoHelpFunc ftnNum, Void*& ppIndirection)
     at Internal.JitInterface.CorInfoImpl._getHelperFtn(IntPtr thisHandle, IntPtr* ppException, CorInfoHelpFunc ftnNum, Void*& ppIndirection)
  --- End of stack trace from previous location where exception was thrown ---
     at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
     at Internal.JitInterface.CorInfoImpl.CompileMethod(MethodCodeNode methodCodeNodeNeedingCode, MethodIL methodIL)
     at ILCompiler.RyuJitCompilation.ComputeDependencyNodeDependencies(List`1 obj)
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeDependencies(List`1 deferredStaticDependencies)
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.ComputeMarkedNodes()
     at ILCompiler.DependencyAnalysisFramework.DependencyAnalyzer`2.get_MarkedNodeList()
     at ILCompiler.RyuJitCompilation.CompileInternal(String outputFile)
     at ILCompiler.Compilation.ILCompiler.ICompilation.Compile(String outputFile)
     at ILCompiler.Program.Run(String[] args)
     at ILCompiler.Program.Main(String[] args)

That CORINFO_HELP_VIRTUAL_FUNC_PTR must be the delegate/interface method stuff.

[MichalStrehovsky]

Presumably this is caused by IntrospectiveSort(keys, index, length, comparer.Compare);

Yup, that's exactly what's going on. Since #2102 is not implemented yet, RyuJIT lands in Compiler::impImportLdvirtftn. On CoreRT we expect RyuJIT to ask for a ReadyToRun helper, but it doesn't seem like CoreCLR defines one when there's runtime lookup involved. We probably want to define a new helper here no matter when #2102 gets implemented to cover for the case of standalone ldvirtftn that is not used in delegate creation.

I'll see if I can cook up something or if I need @sandreenko's help on the RyuJIT side.

Sorry for the trouble.

[mikedn]

I suppose I can take a look at the JIT side as well if you and @sandreenko have other priorities.

[MichalStrehovsky]

@dotnet-bot test this please

[mikedn]

I suppose it's worth asking if such delegate calls have the same perf characteristics they have in the normal CLR. I'm getting the impression that in the current implementation the call will pass through a stub and that more work is needed to avoid this?

[jkotas]

You are right - these calls do not have same perf characteristics in CoreRT right now, but we are going to fix that. #2342 is tracking fixing it. It should not prevent this change from going in.