java.interop icon indicating copy to clipboard operation
java.interop copied to clipboard

Delegate IL size regression in C# 11

Open jonathanpeppers opened this issue 3 years ago • 5 comments
trafficstars

Context: https://github.com/dotnet/roslyn/issues/62832#issuecomment-1198456455

On a recent .NET 7 bump, we found a ~4.5% app size regression related to a new C# 11 delegate feature. I set LangVersion=10 for now to prevent the regression: https://github.com/xamarin/xamarin-android/commit/938b2cbe2f64a0ec0e5984cb59321e05d9db7de7

Instead of emitting e.g.

static Delegate GetGetActionBarHandler ()
{
	if (cb_getActionBar == null)
		cb_getActionBar = JNINativeWrapper.CreateDelegate ((_JniMarshal_PP_L) n_GetActionBar);
	return cb_getActionBar;
}

generator should instead emit:

static Delegate GetGetActionBarHandler ()
{
	if (cb_getActionBar == null)
		cb_getActionBar = JNINativeWrapper.CreateDelegate (new _JniMarshal_PP_L(n_GetActionBar));
	return cb_getActionBar;
}

This should avoid a C#11 "caching method group conversion", allowing Mono.Android.dll assembly size to be ~unchanged between C#10 and C#11.

Workaround

To avoid this size regression, set LangVersion to 10 in your Android bindings project:

<LangVersion>10</LangVersion>

Android bindings projects do not use any features that require C# 11.

jonathanpeppers avatar Jul 28 '22 22:07 jonathanpeppers

See recommendation here: https://github.com/dotnet/roslyn/issues/62832#issuecomment-1201144122

jonathanpeppers avatar Aug 01 '22 13:08 jonathanpeppers

I'm not sure how important this is, i.e. "fix now" or "fix for .NET 8"?

We have a workaround for now -- set $(LangVersion)=10 -- so it doesn't need to be done immediately. We will need to fix this before we can build Mono.Android.dll w/ C# 11.

Other binding projects (AndroidX, GooglePlayServices) will also hit this behavior, and when built against net7.0-android, they will also hit this assembly size regression. However, I don't know how common using net7.0-android as a target framework will be.

jonpryor avatar Sep 01 '22 14:09 jonpryor

I have a feeling we might also want to keep AndroidX/GPS on net6.0-android for a while. Just so we can support both & the .nupkg files won't grow by 50%.

jonathanpeppers avatar Sep 01 '22 14:09 jonathanpeppers

My opinion is that this is too risky a fix to debut in .NET 7 RC2.

We should be good with Mono.Android.dll with the workaround. We currently have no plans to add net7.0-android to AndroidX/GPS/etc. (It would be the same binary as net6.0-android.)

This leaves user bindings that are updated to net7.0-android. We estimate they will see a roughly ~4.5% size increase. I think given how small non-Mono.Android.dll bindings tend to be this should be acceptable for .NET 7. The workaround of setting <LangVersion> to 10 is available if a user wants to recover that size.

One possible alternative is we could default Android Bindings projects to <LangVersion>10</LangVersion> in our .targets? Our generated code does not use any C# 11 specific features.

jpobst avatar Sep 01 '22 14:09 jpobst

I agree that this should not debut in .NET 7 RC2. That's too soon.

It could plausibly debut in some "future service release", e.g. .NET 7.0.200 (or .300, or…). This change would not an ABI change, would not change API, and would not require publishing new packages.

For the time being, scheduling as a .NET 8 feature makes the most sense.

jonpryor avatar Sep 01 '22 16:09 jonpryor