Skip to content

Allow eliminating virtual methods on abstract classes - #80607

Merged
jkotas merged 1 commit into
dotnet:mainfrom
MichalStrehovsky:removeAbstract
Jan 14, 2023
Merged

jkotas merged 1 commit into
dotnet:mainfrom
MichalStrehovsky:removeAbstract

Conversation

@MichalStrehovsky

Copy link
Copy Markdown
Member

Contributes to #80165.

Allow removing method bodies of virtual methods on abstract classes when the methods were overriden by descendants.

E.g.

Base b = new Derived();
b.Expensive();

abstract class Base
{
    public virtual void Expensive() { expensive stuff; }
}

class Derived : Base
{
    public override void Expensive() { other stuff; }
}

The method body of Base.Expensive is unreachable, but we're still generating it because it's referenced from the Base vtable. Not anymore.

This is pretty much a rollback of #66145 with a small fix in ILScanner that fixes the problematic scenarios. I don't know why I gave up so quickly last time.

What we do is instead of generating hard dependencies on the implementation method entrypoints in abstract classes, we delay the dependency on the entrypoint until a derived non-abstract MethodTable. If no non-abstract MethodTable that uses this shows up, the entrypoint turns into a throw helper.

Cc @dotnet/ilc-contrib

Allow removing method bodies of virtual methods on abstract classes when the methods were overriden by descendants.

E.g.

```csharp
Base b = new Derived();
b.Expensive();

abstract class Base
{
    public virtual void Expensive() { expensive stuff; }
}

class Derived : Base
{
    public override void Expensive() { other stuff; }
}
```

The method body of `Base.Expensive` is unreachable, but we're still generating it because it's referenced from the `Base` vtable. Not anymore.

This is pretty much a rollback of dotnet#66145 with a small fix in ILScanner that fixes the problematic scenarios. I don't know why I gave up so quickly last time.

What we do is instead of generating hard dependencies on the implementation method entrypoints in abstract classes, we delay the dependency on the entrypoint until a derived non-abstract MethodTable. If no non-abstract `MethodTable` that uses this shows up, the entrypoint turns into a throw helper.
@ghost

ghost commented Jan 13, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #80165.

Allow removing method bodies of virtual methods on abstract classes when the methods were overriden by descendants.

E.g.

Base b = new Derived();
b.Expensive();

abstract class Base
{
    public virtual void Expensive() { expensive stuff; }
}

class Derived : Base
{
    public override void Expensive() { other stuff; }
}

The method body of Base.Expensive is unreachable, but we're still generating it because it's referenced from the Base vtable. Not anymore.

This is pretty much a rollback of #66145 with a small fix in ILScanner that fixes the problematic scenarios. I don't know why I gave up so quickly last time.

What we do is instead of generating hard dependencies on the implementation method entrypoints in abstract classes, we delay the dependency on the entrypoint until a derived non-abstract MethodTable. If no non-abstract MethodTable that uses this shows up, the entrypoint turns into a throw helper.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@teo-tsirpanis

Copy link
Copy Markdown
Contributor

Doing the same with regular trimming is tracked by dotnet/linker#2910.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

Doing the same with regular trimming is tracked by dotnet/linker#2910.

This PR only does it if the base class is abstract. This is hard enough already :). It's my second attempt after dotnet/runtimelab#658 had to be rolled back. It's hard to do it on non-abstract classes because the base could still be reflection-constructed when one is not paying attention and break this.

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

/asp run runtime-extra-platforms

@EgorBo

EgorBo commented Jan 13, 2023

Copy link
Copy Markdown
Member

@MichalStrehovsky it reminds me that a fix for #54781 might also reduce size (don't compile methods if they're inlined at all callsites) - will take a look this weekend

@MichalStrehovsky

Copy link
Copy Markdown
Member Author

@MichalStrehovsky it reminds me that a fix for #54781 might also reduce size (don't compile methods if they're inlined at all callsites) - will take a look this weekend

I think @jkoritzinsky fixed that one

@jkoritzinsky

Copy link
Copy Markdown
Member

Yes, I fixed that one. It's not 100% perfect (doesn't strip as much as possible when dealing with virtual methods), but it does a decent job

@EgorBo

EgorBo commented Jan 13, 2023

Copy link
Copy Markdown
Member

Yes, I fixed that one. It's not 100% perfect (doesn't strip as much as possible when dealing with virtual methods), but it does a decent job

cool! should the issue be closed then? 🙂

@jkoritzinsky

Copy link
Copy Markdown
Member

I think it's worthwhile to keep it open to track the holes in the analysis (in particular how if a method is an explicit interface implementation of an internal interface method that is never called, it will still be crossgen'd) with the milestone updated to Future as getting the analysis 100% correct is lower priority unless we think it will help us hit size goals for the product.

@jkotas
jkotas merged commit 632f2cd into dotnet:main Jan 14, 2023
@MichalStrehovsky
MichalStrehovsky deleted the removeAbstract branch January 14, 2023 06:33
@ghost ghost locked as resolved and limited conversation to collaborators Feb 13, 2023
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.

5 participants