opened
TIHan
username_0
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.
[WIP] Devirtualize Equality
issue
created
TIHan
username_0
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`.
comment
created
saul
username_1
@username_0 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"
comment
created
TIHan
username_0
@username_1 , 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.
comment
created
TIHan
username_0
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...
comment
created
saul
username_1
But that's already the case right? So this change only improves things - there are no drawbacks from what I can see.
comment
created
TIHan
username_0
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.
comment
created
charlesroddie
username_2
This is great!

Regarding IStructuralEquatable, presumably a generic version would help? https://github.com/dotnet/runtime/issues/37482
comment
created
cartermp
username_3
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.
comment