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.

Sign up for GitHub

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

Jump to bottom

Review preserve lists #5167

Closed
radekdoulik opened this issue Sep 30, 2020 · 2 comments
Closed

Review preserve lists #5167

radekdoulik opened this issue Sep 30, 2020 · 2 comments
Assignees
@radekdoulik
Labels
Area: Linker Issues when linking assemblies.
Milestone
.NET 6

Comments

@radekdoulik
Copy link
Member

Review the preserve lists for illink/net5.

We should also try to use the new illink related attributes where possible.
@radekdoulik
Copy link
Member Author

Also check whether System.ServiceModel is linker safe.

radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Sep 30, 2020
@radekdoulik
Remove System.ServiceModel.xml as well
dc2a5af 
It is not part of .NET core. Added note about it to
dotnet#5167
@jpobst jpobst added this to the One .NET milestone Sep 30, 2020
@jpobst jpobst added the Area: Linker Issues when linking assemblies. label Sep 30, 2020
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 3, 2020
@radekdoulik
[illink] Do not preserve A.R.AndroidEnvironment
b6e993e 
Part of dotnet#5167

Removed API from `BuildReleaseArm64False` test:

    Type Android.Runtime.AndroidEnvironment
      -             Field public static string AndroidLogAppName
      -             Field static Javax.Net.Ssl.IX509TrustManager sslTrustManager
      -             Field static Java.Security.KeyStore certStore
      -             Method static SIGERR SetupTrustManager ()
      -             Method static SIGERR SetupCertStore ()
      -             Method public static SIGERR add_UnhandledExceptionRaiser (System.EventHandler`1<Android.Runtime.RaiseThrowableEventArgs>)
      -             Method public static SIGERR remove_UnhandledExceptionRaiser (System.EventHandler`1<Android.Runtime.RaiseThrowableEventArgs>)
      -             Method static SIGERR TrustEvaluateSsl (System.Collections.Generic.List`1<byte[]>)
      -             Method static SIGERR CertStoreLookup (long, bool)
      -             Method static SIGERR GetX509CertificateFactory ()
      -             Method static SIGERR ConvertCertificate (Java.Security.Cert.CertificateFactory, byte[])
      -             Method static SIGERR NotifyTimeZoneChanged ()
      -             Method static SIGERR GetDisplayDPI (float&, float&)
      -             Method static SIGERR GetDefaultTimeZone ()
      -             Method static SIGERR _monodroid_timezone_get_default_id ()
      -             Method static SIGERR GetDefaultSyncContext ()
      -             Method static SIGERR _monodroid_getifaddrs (IntPtr&)
      -             Method static SIGERR GetInterfaceAddresses (IntPtr&)
      -             Method static SIGERR _monodroid_freeifaddrs (IntPtr)
      -             Method static SIGERR FreeInterfaceAddresses (IntPtr)
      -             Method static SIGERR _monodroid_detect_cpu_and_architecture (ushort&, ushort&, byte&)
      -             Method static SIGERR DetectCPUAndArchitecture (ushort&, ushort&, bool&)
      -             Method static SIGERR GetDefaultProxy ()
      -             Method static SIGERR GetHttpMessageHandler ()
      -             Type _Proxy

Some of the removed API is obsolete on .NET5/6. Opened dotnet#5361
for it.

The template tests run OK on device after the removal.
@radekdoulik radekdoulik mentioned this issue Dec 3, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 3, 2020
@radekdoulik
[illink] Do not always preserve CharSequence
f8eb0fa 
Part of dotnet#5167

Added dynamic dependency where needed.

Removed API from `BuildReleaseArm64False` test:

    Type Android.Runtime.CharSequence
      -             Method public static SIGERR ToLocalJniHandle (System.Collections.Generic.IEnumerable`1<char>)
@radekdoulik radekdoulik mentioned this issue Dec 3, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 3, 2020
@radekdoulik
[illink] Do not preserve ConstructorBuilder
7c8bef2 
Part of dotnet#5167

The type is not accessed by reflection. It can also be made static,
but that is already part of dotnet#5360

Removed API from BuildReleaseArm64False test:

    Type Android.Runtime.ConstructorBuilder
      -             Method public SIGERR .ctor ()
@radekdoulik radekdoulik mentioned this issue Dec 3, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 3, 2020
@radekdoulik
[illink] Do not preserve A.R.Extensions
d4d3ca2 
Part of dotnet#5167

The type is not accessed through reflection.

It is preserved by the linker when needed like:

    --- Method:TResult Android.Runtime.Extensions::JavaCast(Android.Runtime.IJavaObject) dependencies ---
    Dependency dotnet#1
            Method:TResult Android.Runtime.Extensions::JavaCast(Android.Runtime.IJavaObject)
            | MethodSpec:TResult Android.Runtime.Extensions::JavaCast<T>(Android.Runtime.IJavaObject) [1 deps]
            | Method:T Android.App.Activity::FindViewById(System.Int32) [1 deps]
            | MethodSpec:!!0 Android.App.Activity::FindViewById<Android.Widget.Button>(System.Int32) [1 deps]
            | Method:System.Void UnnamedProject.MainActivity::OnCreate(Android.OS.Bundle) [2 deps]
            | Assembly:UnnamedProject, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null [1 deps]
            | Other:Copy
@radekdoulik radekdoulik mentioned this issue Dec 3, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 4, 2020
@radekdoulik
[illink] Do not preserve whole JavaObject
7ff5fc2 
Part of dotnet#5167
@radekdoulik radekdoulik mentioned this issue Dec 4, 2020
Merged
jonpryor pushed a commit that referenced this issue Dec 4, 2020
@radekdoulik
[One .NET] Do not always preserve Android.Runtime.Extensions (#5365)
f58b96d 
Context: #5167

The `Android.Runtime.Extensions` type is not accessed via reflection.

It *is* preserved by the linker when needed, e.g.:

	$ illinkanalyzer -r Android.Runtime.Extensions linker-dependencies.xml.gz
	…
	--- Method:TResult Android.Runtime.Extensions::JavaCast(Android.Runtime.IJavaObject) dependencies ---
	Dependency #1
	        Method:TResult Android.Runtime.Extensions::JavaCast(Android.Runtime.IJavaObject)
	        | MethodSpec:TResult Android.Runtime.Extensions::JavaCast<T>(Android.Runtime.IJavaObject) [1 deps]
	        | Method:T Android.App.Activity::FindViewById(System.Int32) [1 deps]
	        | MethodSpec:!!0 Android.App.Activity::FindViewById<Android.Widget.Button>(System.Int32) [1 deps]
	        | Method:System.Void UnnamedProject.MainActivity::OnCreate(Android.OS.Bundle) [2 deps]
	        | Assembly:UnnamedProject, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null [1 deps]
	        | Other:Copy
	…
jonpryor pushed a commit that referenced this issue Dec 4, 2020
@radekdoulik
[One .NET] Do not always preserve Android.Runtime.CharSequence (#5363)
a74fe4a 
Context: #5167

The `Android.Runtime.CharSequence` type is not accessed via reflection.

Allowing `CharSequence` to be linked allows unreferenced members to
be removed by the linker (yay!), e.g. when comparing the `.apk` built
from the `BuildReleaseArm64False` test:

	$ apkdiff before.apk after.apk
	…
	Type Android.Runtime.CharSequence
	  -             Method public static SIGERR ToLocalJniHandle (System.Collections.Generic.IEnumerable`1<char>)
	…

The `Android.Runtime.CharSequence.ToLocalJniHandle()` is removed.
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 9, 2020
@radekdoulik
[illink] Do not preserve whole JNIEnv class
99e49e9 
Part of dotnet#5167

Instead preserve just methods accessed from native code.
@radekdoulik radekdoulik mentioned this issue Dec 9, 2020
Merged
jonpryor pushed a commit that referenced this issue Dec 9, 2020
@radekdoulik
[One .NET] Don't always preserve Android.Runtime.ConstructorBuilder (#…
9ad0947 
…5364)

Context: #5167

The `Android.Runtime.ConstructorBuilder` type is not accessed
by reflection, and does not need to be fully preserved by the linker.
jonpryor pushed a commit that referenced this issue Dec 9, 2020
@radekdoulik
[One .NET] Don't always preserve Android.Runtime.AndroidEnvironment (#…
11f0e1b 
…5362)

Context: #5167
Context: #5361

The `Android.Runtime.AndroidEnvironment` type is not accessed by
reflection *in .NET 6*, and thus does not need to be fully preserved
by the linker *in .NET 6*.  (Many of these members *are* used via
reflection from mono/2020-02.)

Allowing `AndroidEnvironment` to be linked allows for multiple
members to be removed, e.g. for the `BuildReleaseArm64False` test:

	Type Android.Runtime.AndroidEnvironment
	  -             Field public static string AndroidLogAppName
	  -             Field static Javax.Net.Ssl.IX509TrustManager sslTrustManager
	  -             Field static Java.Security.KeyStore certStore
	  -             Method static SIGERR SetupTrustManager ()
	  -             Method static SIGERR SetupCertStore ()
	  -             Method public static SIGERR add_UnhandledExceptionRaiser (System.EventHandler`1<Android.Runtime.RaiseThrowableEventArgs>)
	  -             Method public static SIGERR remove_UnhandledExceptionRaiser (System.EventHandler`1<Android.Runtime.RaiseThrowableEventArgs>)
	  -             Method static SIGERR TrustEvaluateSsl (System.Collections.Generic.List`1<byte[]>)
	  -             Method static SIGERR CertStoreLookup (long, bool)
	  -             Method static SIGERR GetX509CertificateFactory ()
	  -             Method static SIGERR ConvertCertificate (Java.Security.Cert.CertificateFactory, byte[])
	  -             Method static SIGERR NotifyTimeZoneChanged ()
	  -             Method static SIGERR GetDisplayDPI (float&, float&)
	  -             Method static SIGERR GetDefaultTimeZone ()
	  -             Method static SIGERR _monodroid_timezone_get_default_id ()
	  -             Method static SIGERR GetDefaultSyncContext ()
	  -             Method static SIGERR _monodroid_getifaddrs (IntPtr&)
	  -             Method static SIGERR GetInterfaceAddresses (IntPtr&)
	  -             Method static SIGERR _monodroid_freeifaddrs (IntPtr)
	  -             Method static SIGERR FreeInterfaceAddresses (IntPtr)
	  -             Method static SIGERR _monodroid_detect_cpu_and_architecture (ushort&, ushort&, byte&)
	  -             Method static SIGERR DetectCPUAndArchitecture (ushort&, ushort&, bool&)
	  -             Method static SIGERR GetDefaultProxy ()
	  -             Method static SIGERR GetHttpMessageHandler ()
	  -             Type _Proxy

The removed API is no longer needed on .NET 5+.

The template tests run OK on device after the removal.
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 9, 2020
@radekdoulik
[illink] Do not preserve whole JNIEnv class
b2050f2 
Part of dotnet#5167

Instead preserve just methods accessed from native code.
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 9, 2020
@radekdoulik
[illink] Do not preserve whole JavaObject
11b430b 
Part of dotnet#5167
jonpryor pushed a commit that referenced this issue Dec 10, 2020
@radekdoulik
[One .NET] Don't preserve all of Android.Runtime.JNIEnv (#5362)
d435a97 
Context: #5167

Some members of `Android.Runtime.JNIEnv` are accessed via Reflection,
and need to be preserved, but that doesn't mean the *entire type*
needs to be preserved.

Update the `<type/>` preservation XML so that only the required
methods are always preserved, allowing the linker to remove other
unused members.
jonpryor pushed a commit that referenced this issue Dec 11, 2020
@radekdoulik
[One .NET] Don't preserve all of Android.Runtime.JavaObject (#5367)
d0df243 
Context: #5167

The `Android.Runtime.JavaObject` type is not accessed by reflection,
and does not need to be fully preserved by the linker.

After allowing `JavaObject` to be linked, we discovered that the
presence of the `JavaObject.Clone()` method pulled in a lot of API,
through the `System.ICloneable` interface.

Removing the `JavaObject.Clone()` method allows us to save more than
900KB in compressed assembly size:

	Summary:
	  -     942,460 Assemblies -43.72% (of 2,155,792)
	  -     947,763 Package size difference -10.50% (of 9,024,291)

Which raises an important question: do we *need* `JavaObject.Clone()`?

We believe the answer is *no*: `JavaObject` is a sealed type; no
*managed* code can call `JavaObject.Clone()`.

This leaves the Java Callable Wrapper, which [*oddly*][0] is `public`,
*and* overrides `java.lang.Object.clone()` as a public method.

Thus, *in theory*, `JavaObject.Clone()` *is* invocable from Java code.

However, *in practice*, to do so the Java code would need to take
a compile-time dependency on `mono.android.jar`, which feels *highly*
unlikely and unusual.  (`java.lang.Object.clone()` is `protected` by
default, and is thus not invocable normally.)

We thus deem it safe to entirely remove `JavaObject.Clone()`.

In the end we are now smaller in assembly size comparing simple XA app
"legacy" and net6:

	Summary:
	  -     356,408 Assemblies -27.69% (of 1,287,165)
	  +   2,646,694 Package size difference 51.43% (of 5,146,413)

And still a bit larger comparing XA XForms app:

	Summary:
	  +   1,001,148 Assemblies 21.01% (of 4,765,097)
	  +   4,013,739 Package size difference 37.75% (of 10,633,264)

[0]: dotnet/java-interop#754
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 22, 2020
@radekdoulik
[illink] Do not preserve whole collection types
56f7474 
Context: dotnet#5167
Context: dotnet#5206

Instead of preserving these types unconditionally, use the new dynamic
dependency attributes

apksize impact on BuildReleaseArm64False test:

    > lapkdiff -f -e dll$ before.apk after.apk
    Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
      -         477 assemblies/Java.Interop.dll
      -         603 assemblies/System.Collections.Concurrent.dll
      -       1,388 assemblies/System.Private.CoreLib.dll
      -      30,291 assemblies/Mono.Android.dll
    Summary:
      -      32,759 Assemblies -4.19% (of 781,837)
      -      32,768 Package size difference -0.43% (of 7,645,409)
@radekdoulik radekdoulik mentioned this issue Dec 22, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 28, 2020
@radekdoulik
[illink] Do not preserve IO stream adapter/invoker
2f374a1 
Context: dotnet#5206
Context: dotnet#5167

They are only accessed through `CallbackCode` class in M.A.Export with
reflection code, which ILLink is able to detect.

This change reduces the apk size in common situation, when M.A.Export is
not used.

apk size comparison, BuildReleaseArm64False test:

    > apkdiff -f -e dll$ before.apk after.apk
    Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
      -          80 assemblies/System.Console.dll
      -         129 assemblies/Java.Interop.dll
      -       1,237 assemblies/System.Private.CoreLib.dll
      -       6,093 assemblies/Mono.Android.dll
    Summary:
      -       7,539 Assemblies -1.01% (of 749,078)
@radekdoulik radekdoulik mentioned this issue Dec 28, 2020
Merged
radekdoulik added a commit to radekdoulik/xamarin-android that referenced this issue Dec 28, 2020
@radekdoulik
[illink] Do not preserve GeneratedEnumAttribute
a35e9ef 
Context: dotnet#5167

The attribute itself doesn't need to be preserved.

I didn't find any usage of the attribute instances during runtime, so
this change removes them during linking.

apk size comparison, BuildReleaseArm64False test:

    > apkdiff -f -e dll$ before.apk after.apk
    Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
      -          39 assemblies/Mono.Android.dll
        +         132 Resource Android.ILLink.ILLink.LinkAttributes.xml
        -             Type Android.Runtime.GeneratedEnumAttribute
    Summary:
      -          39 Assemblies -0.01% (of 749,078)

Note that the attribute instances removal doesn't work yet, we need
net6 illink for that.
@radekdoulik radekdoulik mentioned this issue Dec 28, 2020
Merged
jonpryor pushed a commit that referenced this issue Jan 4, 2021
@radekdoulik
[One .NET] Do not preserve IO stream adapter/invoker (#5449)
051cad7 
Context: #5167
Context: #5206

The following types are accessed through the `CallbackCode` type
in `src/Mono.Android.Export` via System.Reflection:

  * `Android.Runtime.InputStreamAdapter`
  * `Android.Runtime.InputStreamInvoker`
  * `Android.Runtime.OutputStreamAdapter`
  * `Android.Runtime.OutputStreamInvoker`

Historically the use of these types via Reflection required that they
always be preserved, just in case `Mono.Android.Export.dll` was used.

.NET 6 ILLink is able to detect such reflection use.  These types no
longer need to be explicitly preserved in all circumstances.

Stop explicitly preserving these types, and rely on ILLink to
preserve them when `Mono.Android.Export.dll` is used.

This change reduces the `.apk` size in a common situation, when
`Mono.Android.Export.dll` is not used.

apk size comparison, BuildReleaseArm64False test:

	> apkdiff -f -e dll$ before.apk after.apk
	Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
	  -          80 assemblies/System.Console.dll
	  -         129 assemblies/Java.Interop.dll
	  -       1,237 assemblies/System.Private.CoreLib.dll
	  -       6,093 assemblies/Mono.Android.dll
	Summary:
	  -       7,539 Assemblies -1.01% (of 749,078)
jonpryor pushed a commit that referenced this issue Jan 7, 2021
@radekdoulik
[One .NET] Do not preserve GeneratedEnumAttribute (#5450)
55dbc33 
Context: #5167

The `GeneratedEnumAttribute` attribute does not need to be preserved
for runtime behavior.  It is only used for bindings.

Removing the attribute allows apps to be smaller.

`.apk` size comparison, BuildReleaseArm64False test:

	> apkdiff -f -e dll$ before.apk after.apk
	Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
	  -          39 assemblies/Mono.Android.dll
	    +         132 Resource Android.ILLink.ILLink.LinkAttributes.xml
	    -             Type Android.Runtime.GeneratedEnumAttribute
	Summary:
	  -          39 Assemblies -0.01% (of 749,078)

Note that the attribute instances removal doesn't work yet; we need
net6 illink for that.
jonpryor pushed a commit that referenced this issue Jan 7, 2021
@radekdoulik
[One .NET] Do not preserve entire collection types (#5445)
15269f6 
Context: #5167
Context: #5206

Instead of preserving the `Android.Runtime.Java*` collection types
unconditionally, use the new dynamic dependency attributes from
7083eba.

`.apk` size impact on BuildReleaseArm64False test:

	> lapkdiff -f -e dll$ before.apk after.apk
	Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
	  -         477 assemblies/Java.Interop.dll
	  -         603 assemblies/System.Collections.Concurrent.dll
	  -       1,388 assemblies/System.Private.CoreLib.dll
	  -      30,291 assemblies/Mono.Android.dll
	Summary:
	  -      32,759 Assemblies -4.19% (of 781,837)
	  -      32,768 Package size difference -0.43% (of 7,645,409)
@jonathanpeppers
Copy link
Member

I think we did some auditing on our preserve lists.

Closing this, we can track this one instead going forward: #5652

@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@radekdoulik radekdoulik

Labels
Area: Linker Issues when linking assemblies.
Projects
None yet
Milestone
.NET 6
Development

No branches or pull requests
3 participants
@jpobst @jonathanpeppers @radekdoulik