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

Sweep base methods if not required #3097

Open
jtschuster opened this issue Nov 1, 2022 · 2 comments
Open

Sweep base methods if not required #3097

jtschuster opened this issue Nov 1, 2022 · 2 comments

Comments

@jtschuster
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants