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

Mono.Android.dll IL Size increase in .NET 7 Preview 7 & RC 1 #62832

Closed
jonathanpeppers opened this issue Jul 21, 2022 · 10 comments
Closed

Mono.Android.dll IL Size increase in .NET 7 Preview 7 & RC 1 #62832

jonathanpeppers opened this issue Jul 21, 2022 · 10 comments
Labels
Area-Compilers Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jul 21, 2022

Version Used:

Happens in both:

  • .NET 7.0.100-rc.1.22368.2 SDK
  • .NET 7.0.100-preview.7.22370.3 SDK

Noticed this here: dotnet/android#7170

Steps to Reproduce:

  1. Build xamarin-android
  2. Observe the size of Mono.Android.dll. This is a C# "binding" of every Android API.
34502656 Mono.Android.net7.rc1.dll
32999936 Mono.Android.xa.main.dll

When I rain monodis --typedef, I noticed a bunch of new types generated, such as:

[CompilerGenerated]
private static class <>O
{
	public static _JniMarshal_PP_L <0>__n_Sink;

	public static _JniMarshal_PP_L <1>__n_Source;
}

I tried to use ILSpy to understand how these are used:

private static Delegate GetSinkHandler()
{
	if ((object)cb_sink == null)
	{
		cb_sink = JNINativeWrapper.CreateDelegate(new _JniMarshal_PP_L(n_Sink));
	}
	return cb_sink;
}

In this case, the above method is called once and would only allocate a _JniMarshal_PP_L once. Are we paying extra file size for something that will only be called once?

Is there some flag to turn off this feature? (at least for this assembly) Thanks!

Here is before & after, and the monodis output: Mono.Android.zip

Expected Behavior:

Updating the .NET SDK, shouldn't increase the size of Mono.Android.dll noticeably.

Actual Behavior:

Updating the .NET SDK, increases the size of Mono.Android.dll.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 21, 2022
@Youssef1313
Copy link
Member

This is possibly #58288, which implements #5835, which is a C# 11 feature.

@Youssef1313
Copy link
Member

I can imagine we can get rid of the generated <>O private classes and emit the fields in the original class. This should reduce the size of the generated code.

@jonathanpeppers
Copy link
Member Author

I think we might be setting LangVersion=preview in this project, let me check what the value is.

@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Jul 21, 2022

@Youssef1313 yes, looks like we are setting LangVersion=latest repo-wide. I think LangVersion=10, will workaround for this one project for now.

The pattern I see above is common for Android "Java binding" projects. Is there a way to turn off this one language feature, and still use C# 11+?

I could see this being a problem for other Java libraries we bind, like AndroidX or Google Play Services -- or customers' own binding projects.

@Youssef1313
Copy link
Member

I don't believe there is currently a way to disable specific features. I think first thing to consider is improving the generated code to be smaller, then check if the size is still problematic or not.

@jonathanpeppers
Copy link
Member Author

I didn't particularly see anything in the above <>O class that could be smaller. Maybe there is?

The problem with the Mono.Android.dll assembly above is the amount of code inside -- it's literally every API from Android. In a 30+ MB assembly, we tend to notice every small change.

jonathanpeppers added a commit to dotnet/android that referenced this issue Jul 22, 2022
Changes: dotnet/installer@85a0482...a9c056c
Changes: dotnet/linker@ef2d0f2...d27ff61
Changes: dotnet/runtime@206dccb...072eda8
Changes: dotnet/emsdk@40e7c62...11a9acf

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22361.1 to 7.0.100-rc.1.22368.2
* Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22365.1
* Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-rc.1.22366.5
* Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-rc.1.22362.2

~~ Set `$(TrimMode)` `partial` by default (#7132) ~~

Companion to dotnet/linker#2856

* Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
* Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full`
* Update .apkdesc files

~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~

There appears to be a C# 11 IL size regression in:

dotnet/roslyn#62832

We can use C# 10 for now to avoid this.

* Fixed `Mono.Android.dll` size in `.apkdesc` files

Co-authored-by: Andy Gocke <andy@commentout.net>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
jonathanpeppers added a commit to dotnet/android that referenced this issue Jul 22, 2022
Changes: dotnet/installer@d2fff6d...dbdda95
Changes: dotnet/linker@ef2d0f2...33a76b8
Changes: dotnet/runtime@206dccb...db5d4df
Changes: dotnet/emsdk@40e7c62...7d27778

Updates:

* Microsoft.Dotnet.Sdk.Internal: from 7.0.100-preview.7.22362.31 to 7.0.100-preview.7.22370.3
* Microsoft.NET.ILLink.Tasks: from 7.0.100-1.22354.1 to 7.0.100-1.22362.1
* Microsoft.NETCore.App.Ref: from 7.0.0-preview.6.22356.1 to 7.0.0-preview.7.22369.4
* Microsoft.NET.Workload.Emscripten.Manifest-7.0.100: from 7.0.0-preview.7.22330.1 to 7.0.0-preview.7.22361.2

~~ Set `$(TrimMode)` `partial` by default (#7132) ~~

Companion to dotnet/linker#2856

* Update src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
* Remove usages of `%(TrimMode)` for `$(AndroidLinkMode)` of `Full`
* Update .apkdesc files

~~ Set `$(LangVersion)` to 10 in Mono.Android.csproj ~~

There appears to be a C# 11 IL size regression in:

dotnet/roslyn#62832

We can use C# 10 for now to avoid this.

* Fixed `Mono.Android.dll` size in `.apkdesc` files

Co-authored-by: Andy Gocke <andy@commentout.net>
Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
@jcouv
Copy link
Member

jcouv commented Jul 28, 2022

FYI @AlekseyTs as feature champion for "Use a cached delegate for method group conversion" and FYI @jaredpar for ecosystem impact. Closing as by-design

@jcouv jcouv closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2022
@jcouv jcouv added Resolution-By Design The behavior reported in the issue matches the current design and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 28, 2022
@jcouv jcouv added this to the 17.4 milestone Jul 28, 2022
@jonathanpeppers
Copy link
Member Author

Yeah my main concern here was:

In this case, the above method is called once and would only allocate a _JniMarshal_PP_L once. Are we paying extra file size for something that will only be called once?

Is there a way to detect this case?

It doesn't sound great, but I guess we can keep Mono.Android.dll on C# 10 indefinitely?

@marek-safar
Copy link
Contributor

I guess you could also rewrite your C# generator not to use method group conversion and control it that way.

@jaredpar
Copy link
Member

jaredpar commented Aug 1, 2022

It doesn't sound great, but I guess we can keep Mono.Android.dll on C# 10 indefinitely?

If you don't want implicit method group caching then you can use an explicit delegate allocation.

Action a1 = Method; // implicit conversion, subject to caching 
Action a2 = new Action(Method); // explicit allocation, never cached 

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 to dotnet/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: #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 to dotnet/java-interop 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

5 participants