dotnet / linker

388 stars 126 forks source link

Sweep base methods if not required #3097

Open jtschuster opened 1 year ago

jtschuster commented 1 year ago

The linker marks all base methods of any marked method, even if it's not strictly required.

For example, in Inheritance.VirtualMethods.VirtualMethodGetsStrippedIfImplementingMethodGetsInvokedDirectly.cs:

class VirtualMethodGetsStrippedIfImplementingMethodGetsInvokedDirectly
    {
        public static void Main ()
        {
            new A ().Foo ();
        }
        [KeptMember (".ctor()")]
        class B
        {
            // TODO: Would be nice to be removed
            [KeptBy (typeof (A), nameof (A.Foo), "BaseMethod")]
            public virtual void Foo ()
            {
            }
        }
        [KeptMember (".ctor()")]
        [KeptBaseType (typeof (B))]
        class A : B
        {
            [KeptBy (typeof(A), nameof(Foo), DependencyKind.DirectCall)]
            public override void Foo ()
            {
            }
        }
    }
}

We know that Foo is only called on A, so we could be able to remove B.Foo() and make A.Foo not virtual.

Similarly, we could remove all of B if we sweep unused base types.

vitek-karas commented 1 year ago

This might be tricky due to how compilers generate calls. In the above the call new A().Foo() is likely going to be translated as callvirt B.Foo - since compilers generates call to the base method whenever possible. The only place where that's not the case if there's return value covariance I think.

We could rewrite the callsite as well, but it makes this more complicated.

MichalStrehovsky commented 1 year ago

NativeAOT actually does extra work to keep the base overrides because I didn't want to deal with all the corner cases that removing the method can have. For example, if the base method has an attribute that sets AttributeUsageAttribute.Inherited property, those attributes should be visible when reflecting on attributes of the overriding method. There might be more. My suggestion would be to be very cautious and also fix #1187 first because if one leans into virtual method resolution within illink too much, it will break a lot more than it breaks right now.

Similarly, we could remove all of B if we sweep unused base types.

This would require marking Type.BaseType as RUC. This one might be used too much to be really feasible.