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

Delegate IL size regression in C# 11 #1034

Closed
jonathanpeppers opened this issue Jul 28, 2022 · 5 comments · Fixed by #1053
Closed

Delegate IL size regression in C# 11 #1034

jonathanpeppers opened this issue Jul 28, 2022 · 5 comments · Fixed by #1053
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Milestone

Comments

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jul 28, 2022

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. I set LangVersion=10 for now to prevent the regression: dotnet/android@938b2cb

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

See recommendation here: dotnet/roslyn#62832 (comment)

@jonpryor jonpryor transferred this issue from dotnet/android Sep 1, 2022
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) and removed needs-triage labels Sep 1, 2022
@jpobst jpobst removed their assignment Sep 1, 2022
@jpobst jpobst added this to the 8.0.0 milestone Sep 1, 2022
@jonpryor jonpryor changed the title Mono.Android.dll, C# 11, and .NET 7 generator should use new DelegateType(method), not (DelegateType) method Sep 1, 2022
@jpobst jpobst changed the title generator should use new DelegateType(method), not (DelegateType) method Delegate IL size regression in C# 11 Sep 1, 2022
@jonpryor
Copy link
Member

jonpryor commented Sep 1, 2022

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.

@jonathanpeppers
Copy link
Member Author

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%.

@jpobst
Copy link
Contributor

jpobst commented Sep 1, 2022

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.

@jonpryor
Copy link
Member

jonpryor commented Sep 1, 2022

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.

@jpobst jpobst removed their assignment Sep 1, 2022
jonpryor added a commit to jonpryor/java.interop that referenced this issue Oct 11, 2022
Context: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-11#improved-method-group-conversion-to-delegate
Context: dotnet#1034
Context: dotnet/roslyn#62832

C#11 introduced a "slight semantic" change with "Improved method
group conversion to delegate":

> The C# 11 compiler caches the delegate object created from a method
> group conversion and reuses that single delegate object.

The result of optimization is that for current `generator`-emitted
code such as:

	static Delegate GetFooHandler ()
	{
	  if (cb_foo == null)
	    cb_foo = JNINativeWrapper.CreateDelegate ((_JniMarshal_PP_V) n_Foo);
	  return cb_foo;
	}

The `(_JniMarshal_PP_V) n_Foo` expression is a "method group
conversion", and under C#11 the generated IL is *larger*, as the
delegate instance is *cached* in case it is needed again.

However in *our* case we *know* the delegate instance won't be needed
again, not in this scope, so all this "optimization" does *for us* is
increase the size of our binding assemblies when built under  C#11.

Review `src/Java.Interop` for use of `new JniNativeMethodRegistration`
and replace "cast-style" method group conversions `(D) M` to
"new-style" delegate conversions `new D(M)`.  This explicitly
"opts-out" of the C#11 optimization.
jonpryor added a commit that referenced this issue Oct 11, 2022
Context: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-11#improved-method-group-conversion-to-delegate
Context: #1034
Context: dotnet/roslyn#62832

C#11 introduced a "slight semantic" change with "Improved method
group conversion to delegate":

> The C# 11 compiler caches the delegate object created from a method
> group conversion and reuses that single delegate object.

The result of optimization is that for current `generator`-emitted
code such as:

	static Delegate GetFooHandler ()
	{
	  if (cb_foo == null)
	    cb_foo = JNINativeWrapper.CreateDelegate ((_JniMarshal_PP_V) n_Foo);
	  return cb_foo;
	}

The `(_JniMarshal_PP_V) n_Foo` expression is a "method group
conversion", and under C#11 the generated IL is *larger*, as the
delegate instance is *cached* in case it is needed again.

However in *our* case we *know* the delegate instance won't be needed
again, not in this scope, so all this "optimization" does *for us* is
increase the size of our binding assemblies when built under  C#11.

Review `src/Java.Interop` for use of `new JniNativeMethodRegistration`
and replace "cast-style" method group conversions `(D) M` to
"new-style" delegate conversions `new D(M)`.  This explicitly
"opts-out" of the C#11 optimization.
jonpryor pushed a commit that referenced this issue Oct 25, 2022
Fixes: #1034

Context: dotnet/roslyn#62832 (comment)
Context: 
Context: dotnet/android@938b2cb

[The C#11 compiler was updated][0] so that "method group conversions"
are now cached:

> The C# 11 compiler caches the delegate object created from a method
> group conversion and reuses that single delegate object.

Our marshal method infrastructure uses method group conversions,
e.g. the cast to `(_JniMarshal_PP_L)` is a method group conversion:

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

This C# 11 compiler change resulted in `Mono.Android.dll` and
.NET Android apps being ~4.5% *larger*.

This was worked around in dotnet/android@938b2cbe by setting
`$(LangVersion)`=10 (i.e. "don't use C# 11").

Update `generator` output to avoid use of method group conversion
for delegate types.  This allows us to use C# 11 without increasing
the size of `Mono.Android.dll` and .NET Android apps:

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

[0]: https://learn.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-11#improved-method-group-conversion-to-delegate
jonpryor pushed a commit to dotnet/android that referenced this issue 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 <jonathan.pobst@microsoft.com>
@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
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants