-
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
Set: comparer optimization #10861
Set: comparer optimization #10861
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.
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
Cannot yet understand why the build fails or reproduce locally with meaningful output. Locally all but 10 tests from FSharp.Core do pass. 10 tests, e.g. Does F# spec require |
Re-triggering CI to see if there's something wrong environmentally, every CI leg shows an NRE when building the boostrap compiler which would be unrelated to your changes... |
@buybackoff yes, there are I think a few tests that I think do depend on some implementation details, in comparersregression.fs. It's a ridiculously large file that's generated by a tool, and consequently, kind of hard to diagnose :( @dsyme could speak more towards the F# spec in this space. |
In the last commit I've forced Set.compare to return [-1;0;1] and all tests for FSharp.Core pass locally
It is related. I've isolated the problem to these lines: fsharp/src/fsharp/FSharp.Core/mapsetcmp.fs Lines 26 to 30 in 0900934
If I add
|
Will close this out as per #10855 (comment) (in case folks don't click the link, the rationale is that regressing reference types here could make the tradeoff not worth it, at least not without more information that it's the right tradeoff.) |
Same as #10855
The job
Main50
includes previous PR