Special-Case Ascending Value Types for System.Linq.OrderBy #336
Conversation
|
@manofstick, thanks for the PRs. I just returned from vacation. I will review your two PRs hopefully this week. |
|
@stephentoub - No rush; just seeking an update... |
|
@manofstick, I'm very sorry for the super-long delay on this one; sincere apologies. In the end, I think we need to go ahead and close it. I was keeping it open in hopes I could convince myself that we should take it, and it's great that it makes the common case with many elements faster, but it's pretty common with LINQ to process short enumerables and to do so repeatedly, and so a regression in a short list in favor of improvements in a long list isn't necessarily a good tradeoff. But really the clincher for me is the long discussion that led to #36643, where multiple folks in the community expressed strong concern at adding an O(N) amount of work even when doing so was part of removing an O(N log N) amount of work; this has a very similar feel to it, and I don't think the rewards associated with this PR are high enough to want to take such a risk again. Thank you, again, for all of your efforts here. |
|
@stephentoub no worries :-) pretty hard to push anything through when there is a mature code base! I mean I'm not 100% convinced that the arguments from #36643 really are relevant here (i.e. the predicate in that case was being used in places where it previously wasn't being used - i.e. slurped up into being called on all elements, rather than just the front elements of a sorted list - i.e. .net frameworks use has a deterministic state, i.e. the sorted first n elements until an event, but in PR it's just being used as an existing sort predicate where any assumptions of use by the client shouldn't exist (Otherwise any change to the sort algo would destroy clients, and I believe in the move to .net core as it moved from a vanilla quick sort to a slight modification, hence changing the potential order of the execution of the predicate, but that didn't cause issues)... ...but regardless, I can easily see from your perspective why this is a risk that you are unwilling to take. Thanks for taking the time to have a look - who knows, I might even hit you with another PR soon :-P ! (there is one more from Cistern that I think could stand a chance!!) |
ghost
locked as resolved and limited conversation to collaborators
4 participants
2nd of n optimizations lifted from my exploration of an alternative
System.LinqimplementationCistern.Linq. There are lots more goodies there, but only so many that will survive the transfer toSystem.Linq!(Previous corefx optimization)
This one (to a first order approximation) adds O(N) operations to the sort (huh?) and also forces dual arrays to be sorted (via
Array.Sort(keys, elements, ...)) so that doesn't sound very good now does it? (Of course it remains a stable sort)But...
Anyway, here are some results from a slightly modified performance test that just uses a variety of sorting sizes gives the following results:
So there is some degraded performance for non-value types for very small collections, but these are minor, and only for < 10 elements - due to additional overhead of checking type, etc., but the performance of a known value type (
DateTime) is significant - ~35% of the time for 256 elements.A value type which has a custom comparer also benefits (although by a much more modest amount). (i.e. not just limited to the case of Array Sort optimizing the primitive types)
But!! If the comparer was significantly complex then this could slow it down (due to the additional O(N) operations) but as N increases, the ~O(N log N) will drown this out.
... and this really isn't just for value types as N grows it overcomes the additional O(N) and has faster results on reference types (well I tried it with the name strings as per
System.Linq.Tests.Perf_OrderBy.OrderByStringand around the few hundred items it was faster). But the current state of this PR is just for value types.