-
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
Generate new Equals overload to avoid boxing for structural comparison #16857
Conversation
❗ Release notes required
|
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
b0a93dd
to
918870a
Compare
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
(I am assuming this is ready for review and merge now) |
The baselines are driving me crazy here, I wanted to open it for review once they are ready. But otherwise yes, all the actual changes I wanted to have here are in. |
...s/EmittedIL/CCtorDUWithMember/CCtorDUWithMember01a.fs.RealInternalSignatureOff.il.net472.bsl
Show resolved
Hide resolved
f45f8ef
to
22c58bc
Compare
Should it also cover struct records? |
@auduchinok yep, it does. At that point, I just hadn't finished with updating the PR description. Now it's there :) |
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.
Fantastic work!
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.
Looks good
@psfinaki Am I missing something, or this change isn't guarded by a language version check? |
No, there was a discussion and eventually we decided not to put it here. I am trying to remember the motivation though. |
The generated members are visible to the outside code, from the analysis point of view this may be a significant change, especially in language interop scenarios, which should normally be guarded by a language version. |
@auduchinok I believe the motivation was that it is not worth the effort (it's doable but given the amount of the affected baselines it will be quite messy). What are the particular downsides of not having it here? We use lang versions to allow teams using mixed versions of F# on their machines. So the product code would use stable F# where the devs could use the latest version if needed. This change doesn't affect semantics much and can be seen as an impl detail - albeit a large one. |
We need to provide info about generated F# symbols to C# and other languages analysis, and this should be inline with what gets compiled. This analysis does boxing checking, among other things, so it has to know about these new members too, but only when they do actually exist. Language versions is what allows to use different rules properly on such changes to the language. |
This PR seems to address this draft RFC (fsharp/fslang-design#747), even though that RFC suggested a slightly different approach (that is, it also addresses the I'm not quite sure how much overlap there is between that one and this PR, perhaps we should align them so that we can publish this amazing PR with the accompanying RFC. |
@abelbraaksma thanks for your nice words :) So yeah this hasn't touched the existing NaN inconsistency - it is still inconsistent. The approach taken in this PR is also substantially different from previous attempts to reduce boxing, but that's because those attempts tried to remove boxing in a different place - basically to use a less generic (and less boxing) comparer. This was finally done earlier this year here - although not including all the original ideas so there is much more to be done in that space. Anyway, the issue which is focused on the NaN problem is here, I added some summary there now. |
I believe I'm misunderstanding things, but won't you be able to tell if it's generated by just getting all methods of the type? How having it behind language version will change the analysis. Can you give an example what will not work or work differently? |
TL;DR
Stop boxing when doing equality test on structs.
And I believe this closes #526. The discussion kind of diverged there but all in all this eliminates the initial problem mentioned, which is also by far the most common usecase of those discussed in the ticket.
What's the problem?
Let's define a struct and compare its two instances:
What's going on here?
For
SomeStruct
, we generate a ctor and implementations for a few handy interfaces:Generated augmentations
The comparison itself is optimized to this:
Which
Equals
of the generated ones is used? We have three:Despite the fact that we know the "exact" type of compared things (as in the second overload), we have to call the first one due to the fact it's the only one with two parameters. And since it's first argument is object, there is boxing happening:
The problematic operation is at
IL_0022
. This is not good - doing redundant operations takes unnecessary time and memory.What's the solution?
We now generate a new
Equals
overload to be called in these cases. It has the "exact" type instead of the object type as an argument:Besides, the other two-parameter overload now calls this one, similar to how it is happening with the one-parameter overloads. Here is how things look like:
Generated augmentations
Now, for the equality test, the generate code looks exactly the same:
However, it's a different overload that is called now, hence the IL differs:
There is no more boxing involved in here. This is confirmed by benchmarks:
Where else does it help?
This also applies to struct unions and struct records in the same fashion.
Before:
After:
This also applies to the generic versions of the above.¨
Before:
After:
In case of one-member constructs, the comparison gets inlined. E.g. consider this code:
The generated comparison used to be:
Now it is simplified to this:
This leads to even bigger speed reductions in all such cases.
Before:
After:
Practical applications
From simple equality tests to real world scenarios.
Here are some examples for array functions, where equality is involved. The bigger the array is, and the more equality tests are performed, the higher are the gains. The following are variations an "exists" functionality, the optimistic and pessimistic cases.
Before:
After:
The feature also works cross-assembly. Among other things, this means that equality tests on F# builtin struct types also become faster and less-allocating.
Before:
After:
Implementation
AugmentWithHashCompare.fs
Optimizer.fs
ExactEquals.fs
files inMicroPerf