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

Don't use interfaces for sharing Tuple implementation methods #44684

Merged
merged 1 commit into from
Nov 17, 2020

Conversation

marek-safar
Copy link
Contributor

Tuples add a dependency on rarely used interfaces which block them from
trimming everywhere. Instead of virtual call over interface share
the implementation as a separate method.

There is a corner case change in behaviour.

As Tuple classes are not sealed, it was possible to override them with
another custom IStructuralEquatable implementation. The object-based
Equals implementation would end up calling custom explicit interface
which I guess is unintended behaviour. The same would happen with
IStructuralComparable.

@stephentoub @jkotas (I'll fix the remaining Tuples if there are no objections)

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@jkotas
Copy link
Member

jkotas commented Nov 14, 2020

What does pull System.Tuple into the dependency closure in typical apps? I thought that they are really only used by F#.

@marek-safar
Copy link
Contributor Author

What does pull System.Tuple into the dependency closure in typical apps?

Tuples are still used even inside SPC. I looked at replacing them which would help too but there is no good replacement as they are used at places where valuetuples would have to go via object boxing/unboxing

@jkotas
Copy link
Member

jkotas commented Nov 14, 2020

@dsyme Any concerns about this change from the F#?

@jkotas
Copy link
Member

jkotas commented Nov 14, 2020

I would be nice to include some numbers for these type of changes in PR descriptions, e.g. how much smaller trimmed "Hello world" gets so we can collectively develop better understanding how much different patterns cost.

@huoyaoyuan
Copy link
Member

I looked at replacing them which would help too but there is no good replacement as they are used at places where valuetuples would have to go via object boxing/unboxing

I wonder if record is suitable here.

@jkotas
Copy link
Member

jkotas commented Nov 15, 2020

I do not think records would be good alternative. Records come with a quite a bit of baggage, some of which would be impossible to trim. The C# auto-implements about 10 methods for record. It is about 10x more than what one needs to pass a tuple of values around.

I think the light weight alternative for the current usage of Tuple in CoreLib would look like this:

    class LightTuple<T1, T2>
    {
        public T1 Item1;
        public T2 Item2;
    }

And it would be used like this:

        public virtual Task WriteLineAsync(char value)
        {
            var tuple = new LightTuple<TextWriter, char>(){ Item1 = this, Item2 = value };
            return Task.Factory.StartNew(static state =>
...

@marek-safar
Copy link
Contributor Author

I would be nice to include some numbers for these type of changes in PR descriptions

This case is a bit tricky to estimate. For hello-world it's about 3kb, for real app it can be probably 10kb or even more as it depends on how many types which implement such interfaces will exist (that includes other tuple types as well).

I think the light weight alternative for the current usage of Tuple in CoreLib would look like this:

Yeah, I was wondering if introducing internal tuple like types for a few payloads we need them is better alternative (we did something similar in Mono).

@dsyme
Copy link

dsyme commented Nov 15, 2020

@dsyme Any concerns about this change from the F#?

I looked it over and it seems semantics are precisely preserved, there is no actual semantic change here, and we're just avoiding an unnecessary dispatch via IStructuralEquatable, which can only be good from the F# perspective

Tuples add a dependency on rarely used interfaces which block them from
trimming everywhere. Instead of virtual call over interface share
the implementation as a separate method.

There is a corner case change in behaviour.

As Tuple classes are not sealed, it was possible to override them with
another custom IStructuralEquatable implementation. The object-based
Equals implementation would end up calling custom explicit interface
which I guess is unintended behaviour. The same would happen with
IStructuralComparable.
@marek-safar marek-safar merged commit 583a536 into dotnet:master Nov 17, 2020
@marek-safar marek-safar deleted the res branch November 17, 2020 12:06
@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants