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

Obsolete PreserveAttribute #5664

Closed
marek-safar opened this issue Feb 26, 2021 · 0 comments · Fixed by #6129
Closed

Obsolete PreserveAttribute #5664

marek-safar opened this issue Feb 26, 2021 · 0 comments · Fixed by #6129
Milestone

Comments

@marek-safar
Copy link
Contributor

The attribute is not needed in .NET6 and can interfere with supported .NET linker attributes. The linker is able to recognize and mark reflection patterns automatically and for custom scenarios better attribute is supported https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.dynamicdependencyattribute.

@jpobst jpobst added this to the One .NET milestone Mar 8, 2021
jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Jul 26, 2021
Fixes: dotnet#5664

The .NET 5+ replacement for [`Android.Runtime.PreserveAttribute`][0]
is [`System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute`][1].

One "discrepancy" is that the relationship is "backwards":
`[Preserve]` is an *assertion* that the member is required, and that
the member should be preserved by the linker.

`DynamicDependencyAttribute` is an indicator: *if* the member with
`[DynamicDependency]` is preserved, then the members referenced by
`[DynamicDependency]` will be preserved.

Consider commit 15269f6: in a `[Preserve]` world, we had:

	// `Mono.Android.dll`
	[Preserve(AllMembers=true)] partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	[Preserve(AllMembers=true)] partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

This setup meant that even when `Mono.Android.Export.dll` *wasn't*
used, all members of `JavaCollection` and `JavaDictionary` were
preserved by the linker, increasing `.apk` size.

With a `[DynamicDependency]` world in commit 15269f6, we now have:

	// `Mono.Android.dll`
	partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaCollection", "Mono.Android")]
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaDictionary", "Mono.Android")]
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

Now, if `Mono.Android.Export.dll` *isn't* used, then `JavaCollection`
and `JavaDictionary` can be removed, enabling smaller apps:

	> apkdiff -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)

[0]: https://docs.microsoft.com/en-us/dotnet/api/foundation.preserveattribute?view=xamarin-ios-sdk-12
[1]: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.dynamicdependencyattribute?view=net-5.0
jonpryor added a commit that referenced this issue Jul 27, 2021
Fixes: #5664

The .NET 5+ replacement for [`Android.Runtime.PreserveAttribute`][0]
is [`System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute`][1].

One "discrepancy" is that the relationship is "backwards":
`[Preserve]` is an *assertion* that the member is required, and that
the member should be preserved by the linker.

`DynamicDependencyAttribute` is an indicator: *if* the member with
`[DynamicDependency]` is preserved, then the members referenced by
`[DynamicDependency]` will be preserved.

Consider commit 15269f6: in a `[Preserve]` world, we had:

	// `Mono.Android.dll`
	[Preserve(AllMembers=true)] partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	[Preserve(AllMembers=true)] partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

This setup meant that even when `Mono.Android.Export.dll` *wasn't*
used, all members of `JavaCollection` and `JavaDictionary` were
preserved by the linker, increasing `.apk` size.

With a `[DynamicDependency]` world in commit 15269f6, we now have:

	// `Mono.Android.dll`
	partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaCollection", "Mono.Android")]
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaDictionary", "Mono.Android")]
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

Now, if `Mono.Android.Export.dll` *isn't* used, then `JavaCollection`
and `JavaDictionary` can be removed, enabling smaller apps:

	> apkdiff -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)

[0]: https://docs.microsoft.com/en-us/dotnet/api/foundation.preserveattribute?view=xamarin-ios-sdk-12
[1]: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.dynamicdependencyattribute?view=net-5.0
jonpryor added a commit that referenced this issue Jul 27, 2021
Fixes: #5664

The .NET 5+ replacement for [`Android.Runtime.PreserveAttribute`][0]
is [`System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute`][1].

One "discrepancy" is that the relationship is "backwards":
`[Preserve]` is an *assertion* that the member is required, and that
the member should be preserved by the linker.

`DynamicDependencyAttribute` is an indicator: *if* the member with
`[DynamicDependency]` is preserved, then the members referenced by
`[DynamicDependency]` will be preserved.

Consider commit 15269f6: in a `[Preserve]` world, we had:

	// `Mono.Android.dll`
	[Preserve(AllMembers=true)] partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	[Preserve(AllMembers=true)] partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

This setup meant that even when `Mono.Android.Export.dll` *wasn't*
used, all members of `JavaCollection` and `JavaDictionary` were
preserved by the linker, increasing `.apk` size.

With a `[DynamicDependency]` world in commit 15269f6, we now have:

	// `Mono.Android.dll`
	partial class JavaCollection {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	partial class JavaDictionary {
	    public static IntPtr ToLocalJniHandle (ICollection? items) {…}
	}
	// …

	// `Mono.Android.Export.dll`
	partial class DynamicInvokeTypeInfo {
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaCollection", "Mono.Android")]
	    [DynamicDependency ("ToLocalJniHandle", "Android.Runtime.JavaDictionary", "Mono.Android")]
	    public CodeExpression ToNative (CodeExpression arg) {
	        switch (GetKind (type)) {
	            case SymbolKind.Collection: return new CodeMethodCall (type.GetMethod ("ToLocalJniHandle"), arg);
	        }
	    }
	}

Now, if `Mono.Android.Export.dll` *isn't* used, then `JavaCollection`
and `JavaDictionary` can be removed, enabling smaller apps:

	> apkdiff -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)

[0]: https://docs.microsoft.com/en-us/dotnet/api/foundation.preserveattribute?view=xamarin-ios-sdk-12
[1]: https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.dynamicdependencyattribute?view=net-5.0
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants