-
Notifications
You must be signed in to change notification settings - Fork 789
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
Map: comparer optimization #10855
Map: comparer optimization #10855
Conversation
Store height in leaves. Compared to the old discussion, when Left/Right were proposed to be stored in a universal node, this adds 4 bytes to leaves or 2 bytes per item on average (vs 16/8).
`Match` produces `sub 1` and `switch` instruction. Here, for any non-trivial count, nodes are more frequent than leaves on the path, so branch prediction should be beneficial.
src/fsharp/FSharp.Core/map.fs
Outdated
|
||
else if Type.op_Equality(typeof<'T>, typeof<bool>) then unbox<bool>(box(l)).CompareTo(unbox<bool>(box(r))) | ||
else if Type.op_Equality(typeof<'T>, typeof<char>) then unbox<char>(box(l)).CompareTo(unbox<char>(box(r))) | ||
else if Type.op_Equality(typeof<'T>, typeof<float>) then unbox<float>(box(l)).CompareTo(unbox<float>(box(r))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
floats have non-standard treatment in F#, should remove these lines or replicate the behavior
This is what we could get if using:
|
Here are the numbers if we only use
But this time delegates are used for ref-type IComparables
Both RefLike and normal record do implement
|
It works quite well now, without static readonly and custom IL. Related to the changes is fsharp/fslang-suggestions#816. We use Strange thing happens with Previous benchmarks were not correct for some cases in absolute terms (e.g. wrong number of
|
Inline primitive comparison, keep the rest the same. See Comparison/cmp comments on how and why this works.
Arrays and structural comparison may cause problems Maybe should just copy the snippet from dotnet#9348
…ilable It's unambiguous. If it does something different than other ways to compare then it's very weird. F# records implement IComparable<T> that works as expected.
… to share later with Set
Hmmmm. The regression for reflikes in your testing is troublesome. I can't speak towards overall usage, but within the compiler we definitely make heavy use of reference types and this would negatively affect that. |
Yes, when I was writing "<5% mostly edge cases" of course I was biased towards my own usage patterns... Overall, these things look too complicated. I opened the draft PRs mostly for CI and to show the magnitude of possible improvements. I'm glad that I was able to split the whole Map/Set story into independent steps. If you close these 2 comparer PRs I'm fine with it. A was aware of the comparer inefficiency for a long time. But I'm rather out of the loop these days and when I've learned all the complexity and history of the |
Okay, makes sense. Thanks for the explanation! And for all of the enhancement so far :) |
Copied from comment #10845 (comment) to show the code and run CI
Map/Set always use the default comparison. It should be easy to optimize it... One may think... But I've seen #9348, #513, and related. Such a hopeless state to optimize (inline/devirtualize) the comparison :(
This implementation is probably not 100% compatible with the current behavior. It could be if we apply the logic from #9348 snippet, to exclude records & DUs, if that code is correct.
If we take the current NuGet 5.0 release without any changes to Map as a baseline, are we ready to (
Ratio
column)getItem
,struct T : IComparable<T>
by 4.14x -getItemIntLike
,getItemIntRecord
string
by 52% -getItemString
,getItemRefLike
, 17% for reference type records -getItemIntRefRecord
?
I would say that everything could be wrapped by
struct T : IComparable<T>
+ efficient logic there, and if someone uses ref-types as a key they do care about about performance by definition. But I do understand that regressing existing code by 15/17% may be too big. Yet the tradeoff is so great for primitive types.What do you think? What's the most important use case for the map/set?
The types in the bench are:
isinst
check #10845