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

[generator] Avoid C#11 delegate cache overhead. #1053

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Oct 20, 2022

Fixes: #1034
Context: dotnet/roslyn#62832 (comment)

On a recent .NET 7 bump, we found a ~4.5% app size regression related to a new C# 11 delegate feature.

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.

Test JI bump with this change is green: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6867774&view=results.

@jpobst jpobst marked this pull request as ready for review October 25, 2022 14:13
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

The changes here look right.

The only thing we should do in the next java.interop bump, is delete this line:

https://github.com/xamarin/xamarin-android/blob/ab6712a48e4b93e9c3d605c7b393814d477e2bc8/src/Mono.Android/Mono.Android.csproj#L25-L26

And just let Mono.Android.dll compile with C# 11, checking what the size diff is.

@jpobst
Copy link
Contributor Author

jpobst commented Oct 25, 2022

Yeah, my bad, I should have mentioned my test XA PR does that and no tests fail. 🎉

https://github.com/xamarin/xamarin-android/pull/7478/files

@jonpryor jonpryor merged commit 8e18c90 into main Oct 25, 2022
@jonpryor jonpryor deleted the new-delegate branch October 25, 2022 16:23
jonpryor pushed a commit to dotnet/android that referenced this pull request Nov 1, 2022
Fixes: dotnet/java-interop#1034
Fixes: dotnet/java-interop#1051

Context: 938b2cb

Changes: dotnet/java-interop@e1ee4b1...5318261

  * dotnet/java-interop@53182615: [build] Update Microsoft.* NuGet package versions (dotnet/java-interop#1055)
  * dotnet/java-interop@8e18c909: [generator] Avoid C#11 delegate cache overhead. (dotnet/java-interop#1053)
  * dotnet/java-interop@2d8b6d24: [generator] More AttributeTargets on SupportedOSPlatformAttribute (dotnet/java-interop#1054)
  * dotnet/java-interop@7dfbab67: [generator] Add [SupportedOSPlatform] to bound constant fields (dotnet/java-interop#1038)
  * dotnet/java-interop@1720628a: [generator] Mark generated .cs files as generated (dotnet/java-interop#1052)
  * dotnet/java-interop@f498fcf5: [Java.Interop] Avoid some method group conversions (dotnet/java-interop#1050)
  * dotnet/java-interop@16e1ecd4: [build] Use $(VSINSTALLDIR), not $(VSINSTALLROOT) (dotnet/java-interop#1048)
  * dotnet/java-interop@8e4c7d20: [Hello-Core] Add "low level" sample. (dotnet/java-interop#1047)

Additionally, remove `$(LangVersion)`=10 from `Mono.Android.csproj`,
which was a hack to work around a size regression due to delegate
caching in C# 11; see also 938b2cb, dotnet/java-interop@8e18c909.

Co-authored-by: Jonathan Pobst <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delegate IL size regression in C# 11
3 participants