Skip to content
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

[WIP] Devirtualize Equality #6175

Closed
wants to merge 2 commits into from
Closed

Conversation

TIHan
Copy link
Contributor

@TIHan TIHan commented Feb 1, 2019

This PR optimizes current equality semantics to be devirtualized to IEquatable<'T>.Equals('T) for F# types; this is to avoid boxing for struct types. It also adds --optimize:equality for a even more optimistic approach to devirtualize equality to IEquatable<'T>.Equals('T) for F#/.NET types that are custom equality and don't implement IStructuralEquatable.

This will solve most of the performance problems related to boxing when doing equality on struct types.

However, it is still possible that boxing on equality can occur if the F# type has the [<CustomEquality>] attribute and implements IStructuralEquatable; IStructuralEquatable.Equals(obj, IEqualityComparer) will take priority over Object.Equals(obj)/IEquatable<'T>.Equals('T)) in this case, which means boxing for struct types. The same goes for .NET types. Meaning if a .NET type implements IStructuralEquatable, it will call IStructuralEquatable.Equals(obj, IEqualityComparer) over Object.Equals(obj)/IEquatable<'T>.Equals('T)).

Remaining work:

  • Tests
  • Cleanup
  • Optimize hash to devirtualize to GetHashCode if it makes sense.
  • Conversation with C# about this.

C# may have a future feature with records and it's worth having just a bit of a conversation regarding equality semantics. My fear is that if C# gets struct records, we would still box on doing equality on them.

Also, I want to explore if we could just add a switch to always force Object.Equals(obj)/IEquatable<'T>.Equals('T) no matter if a type implements IStructuralEquatable or not. This way we can always guarantee a non-boxing call for equality, even if it changes semantics. I don't want to rule this out just yet; probably won't happen, but it would be good to note it down here.

@TIHan TIHan modified the milestones: Unknown, 16.1 Feb 1, 2019
@cartermp cartermp modified the milestones: 16.1, 16.2 Apr 23, 2019
@cartermp cartermp modified the milestones: 16.2, Backlog Apr 30, 2019
@TIHan
Copy link
Contributor Author

TIHan commented May 24, 2019

The biggest thing holding me back on this is the discussion with the C# fellows (my fault, not theirs :) ). I also need to speak with @terrajobst of any types in the future that could implement IStructuralEquatable.

@TIHan TIHan self-assigned this May 24, 2019
@saul
Copy link
Contributor

saul commented May 29, 2019

@TIHan this PR would be really useful for performance-sensitive code we're writing atm. Is it possible to move forward without consensus with C#, in a way that won't bite us in the future? I'm specifically referring to the part of the PR which "optimizes current equality semantics to be devirtualized to IEquatable<'T>.Equals('T) for F# types"

@TIHan
Copy link
Contributor Author

TIHan commented May 29, 2019

@saul , I talked with @terrajobst a bit. I'm not as concerned anymore; I just need to think through it. I want to make this right.

This can be a priority for me as I want to get this in for 16.2.

@TIHan
Copy link
Contributor Author

TIHan commented May 29, 2019

A concern I have is System.Collections.Immutable.ImmutableArray<'T>, as it's a struct. It implements both IEquatable<'T> and IStructuralEquatable. With these devirtualization rules for equality, and even the more optimized version of it using the command line arg, --optimize:equality, it will still call IStructuralEquatable which means boxing...

@saul
Copy link
Contributor

saul commented May 29, 2019

it will still call IStructuralEquatable which means boxing...

But that's already the case right? So this change only improves things - there are no drawbacks from what I can see.

@TIHan
Copy link
Contributor Author

TIHan commented May 29, 2019

There would be no drawbacks to going ahead and using the rules that are not driven by the flag, --optimize:equality. The flag changes behavior technically, but retains the same semantics; which is why I want to cover everything.

@charlesroddie
Copy link
Contributor

This is great!

Regarding IStructuralEquatable, presumably a generic version would help? dotnet/runtime#37482

@cartermp cartermp closed this Jun 27, 2020
@cartermp cartermp reopened this Jun 27, 2020
@cartermp cartermp changed the base branch from dev16.0 to master June 27, 2020 17:48
@cartermp
Copy link
Contributor

This is very much out of date and represents an experiment that I don't think we'll be pursuing within the F# 5 timeframe. I'll close this out for now, but it is absolutely something we wish to address fully.

@cartermp cartermp closed this Jun 27, 2020
@cartermp cartermp removed this from the Backlog milestone Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants