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

Optimize away unused virtual implementations on abstract classes #658

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Feb 8, 2021

This change lets us optimize away virtual method bodies on abstract classes that are overriden in all derived classes.

E.g.

class Base
{
    public virtual void DoSomething() => CallExpensiveMethod();
}

class Derived : Base
{
    public override void DoSomething() => SomeOtherMethod();
}

We shouldn't need to generate Base.DoSomething above since nobody calls that.

The optimization reuses the tentative instance methods introduced last week - the first commit just extracts that into a reusable component. The second commit makes us consider references to virtual method on abstract classes tentative, until e.g. a derived class that uses the base implementation makes them not tentative anymore.

We would probably get more bang if we could this to non-abstract classes too, but it's pretty difficult to discern "constructed EEType that is only constructed because it's a base of an allocated type". The biggest complication is reflection.

@MichalStrehovsky MichalStrehovsky added area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Feb 8, 2021
@jkotas
Copy link
Member

jkotas commented Feb 8, 2021

I would be fine with this. If we have the capability, we can also use it where possible - even it means just 0.6% for what you have measured.

@MichalStrehovsky
Copy link
Member Author

I would be fine with this. If we have the capability, we can also use it where possible - even it means just 0.6% for what you have measured.

Sounds good! I'll add the comments and tests tomorrow. Also I broke multifile again so I gotta fix that (I kind of expected that).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

This change lets us optimize away virtual method bodies on abstract classes that are overriden in all derived classes.

E.g.

```csharp
class Base
{
    public virtual void DoSomething() => CallExpensiveMethod();
}

class Derived : Base
{
    public override void DoSomething() => SomeOtherMethod();
}
```

We shouldn't need to generate `Base.DoSomething` above since nobody calls that.

The optimization reuses the tentative instance methods introduced last week - the first commit just extracts that into a reusable component. The second commit makes us consider references to virtual method on abstract classes tentative, until e.g. a derived class that uses the base implementation makes them not tentative anymore.

We would probably get more bang if we could this to non-abstract classes too, but it's pretty difficult to discern "constructed EEType that is only constructed because it's a base of an allocated type". The biggest complication is reflection.
@MichalStrehovsky MichalStrehovsky removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 9, 2021
@MichalStrehovsky
Copy link
Member Author

Who disabled "rebase and merge" and for what reason? Sigh. Merge commit it is.

@MichalStrehovsky MichalStrehovsky merged commit fc8ce2f into dotnet:feature/NativeAOT Feb 10, 2021
@MichalStrehovsky MichalStrehovsky deleted the optimizeUnusedAbstract branch February 10, 2021 08:51
@jkotas
Copy link
Member

jkotas commented Feb 10, 2021

I was asked to disable it to match dotnet/runtime.

@MichalStrehovsky
Copy link
Member Author

I was asked to disable it to match dotnet/runtime.

Sigh. I remember that issue in the runtime repo and it didn't make sense to me at that time and it doesn't make sense to me today after re-reading it either. Yay for consistency.

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Mar 4, 2022
Turns out this optimization is a bit more complicated. Adding a test case for when it didn't do the right thing, in case we want to come back to it later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants