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

[Xamarin.Android.Build.Tasks] implement $(AndroidStripILAfterAOT) #8172

Merged
merged 14 commits into from
Aug 22, 2023
Merged

[Xamarin.Android.Build.Tasks] implement $(AndroidStripILAfterAOT) #8172

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
815e374
[Xamarin.Android.Build.Tasks] implement $(AndroidStripIL)
jonathanpeppers Jul 6, 2023
2d8bfff
Update parameter names
fanyang-mono Aug 11, 2023
d0db340
Disable removing attribute when AndroidStripIL is enanbled.
fanyang-mono Aug 14, 2023
f747395
Merge remote-tracking branch 'origin/main' into peppers/AndroidStripIL
fanyang-mono Aug 16, 2023
19b2ab9
Add a test
fanyang-mono Aug 16, 2023
6152320
Set default parameter values
fanyang-mono Aug 16, 2023
0feee89
Add documentation of the new feature
fanyang-mono Aug 16, 2023
747aabc
Simpler default for AndroidEnableProfiledAot
jonathanpeppers Aug 16, 2023
0902fea
AndroidStripIL can default to empty
jonathanpeppers Aug 16, 2023
6ac4b46
Update parameter name
fanyang-mono Aug 17, 2023
647c8b9
Update build-properties.md
jonpryor Aug 18, 2023
b5dc596
Merge branch 'main' into AndroidStripIL
jonathanpeppers Aug 18, 2023
e4db7b8
EnableAndroidStripILAfterAOT asserts IL is stripped
jonathanpeppers Aug 18, 2023
40475e2
`$(AndroidEnableProfiledAot)` test parameter
jonathanpeppers Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Documentation/guides/building-apps/build-properties.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,15 @@ This is only used when building `system` applications.

Support for this property was added in Xamarin.Android 11.3.

## AndroidStripILAfterAOT

When setting it to true, the method body of the AOT compiled methods will be strip away.
By default, `AndroidStripILAfterAOT` is set to false.
To maximize the impact of this feature, when setting it to true, the default value of
`AndroidEnableProfiledAot` becomes false. However, you could overwrite it by setting it to true explicitly.

Support for this property was added in .NET 8.

## AndroidSupportedAbis

A string property that contains a
Expand Down
14 changes: 14 additions & 0 deletions ...marin.Android.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.Aot.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,25 @@ They run in a context of an inner build with a single $(RuntimeIdentifier).
LLVMPath="$(_LLVMPath)"
LdName="$(_LdName)"
LdFlags="$(_LdFlags)"
CollectTrimmingEligibleMethods="$(AndroidStripILAfterAOT)"
TrimmingEligibleMethodsOutputDirectory="$(IntermediateOutputPath)tokens"
WorkingDirectory="$(MSBuildProjectDirectory)"
AotArguments="$(AndroidAotAdditionalArguments)">
<Output TaskParameter="CompiledAssemblies" ItemName="_MonoAOTCompiledAssemblies" />
<Output TaskParameter="FileWrites" ItemName="FileWrites" />
</MonoAOTCompiler>
<ILStrip
Condition=" '$(AndroidStripILAfterAOT)' == 'true' "
TrimIndividualMethods="true"
Assemblies="@(_MonoAOTCompiledAssemblies)"
DisableParallelStripping="$(_DisableParallelAot)">
<Output TaskParameter="TrimmedAssemblies" ItemName="_ILStripTrimmedAssemblies" />
</ILStrip>
<Move
Copy link
Member

Choose a reason for hiding this comment

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

This <Move/> feels absurd to me: @(_ILStripTrimmedAssemblies) is an output parameter, so I would normally intuit/assume that %(_ILStripTrimmedAssemblies.Identity) is the file that was created. However, this <Move/> implies that %(_ILStripTrimmedAssemblies.Identity) has not in fact been created, and instead what was created is %(_ILStripTrimmedAssemblies.TrimmedAssemblyFileName), which must be moved to %(_ILStripTrimmedAssemblies.Identity).

Does this make sense to anyone? Why was <ILStrip/> defined this way?

Copy link
Member

@fanyang-mono fanyang-mono Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The goal of ILStrip is to trim IL code away from the assemblies which were AOT'ed. However, not all AOT'ed assemblies might be trimmable. That's why we have _ILStripTrimmedAssemblies. ILStrip also doesn't overwrite existing assemblies with the trimmed ones. It is designed this way, in case %(_ILStripTrimmedAssemblies.Identity) are not the ones that you want to replace with the trimmed ones. They could be the ones which live in other directories. And ILStrip could possibly has no knowledge of that.

Here for System.Private.CoreLib.dll,
%(_ILStripTrimmedAssemblies.Identity) is <a_path>/System.Private.CoreLib.dll
%(_ILStripTrimmedAssemblies.TrimmedAssemblyFileName) is <a_path>/System.Private.CoreLib_trimmed.dll

Copy link
Member

Choose a reason for hiding this comment

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

@fanyang-mono: how does <ILStrip/> know which method bodies it can remove? I see that the <MonoAOTCompiler/> invocation was updated to add a MonoAOTCompiler.TrimmingEligibleMethodsOutputDirectory property, but how does <ILStrip/> konw about the $(IntermediateOutputPath)tokens directory?

Copy link
Member

Choose a reason for hiding this comment

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

The output _MonoAOTCompiledAssemblies from MonoAOTCompiler has a metadata MethodTokenFile. It stores the trimmable method token for the corresponding assembly.

Condition=" '$(AndroidStripILAfterAOT)' == 'true' "
SourceFiles="@(_ILStripTrimmedAssemblies->'%(TrimmedAssemblyFileName)')"
DestinationFiles="@(_ILStripTrimmedAssemblies)"
/>
<WriteLinesToFile
File="$(_AndroidStampDirectory)_AndroidAot.stamp"
Lines="@(_MonoAOTCompiledAssemblies->'%(LibraryFile)')"
Expand Down
2 changes: 1 addition & 1 deletion ...Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.DefaultProperties.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@
<_AndroidXA1029 Condition=" '$(AotAssemblies)' != '' ">true</_AndroidXA1029>
<_AndroidXA1030 Condition=" '$(RunAOTCompilation)' == 'true' and '$(PublishTrimmed)' == 'false' ">true</_AndroidXA1030>
<AotAssemblies>$(RunAOTCompilation)</AotAssemblies>
<AndroidEnableProfiledAot Condition=" '$(AndroidEnableProfiledAot)' == '' and '$(RunAOTCompilation)' == 'true' ">true</AndroidEnableProfiledAot>
<AndroidEnableProfiledAot Condition=" '$(AndroidEnableProfiledAot)' == '' and '$(RunAOTCompilation)' == 'true' and '$(AndroidStripILAfterAOT)' != 'true' ">true</AndroidEnableProfiledAot>

<!--
Runtime libraries feature switches defaults
Expand Down
2 changes: 1 addition & 1 deletion src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1973,7 +1973,7 @@ because xbuild doesn't support framework reference assemblies.

<!-- Shrink Mono.Android.dll by removing attribute only needed for GenerateJavaStubs -->
<RemoveRegisterAttribute
Condition="'$(AndroidLinkMode)' != 'None' AND '$(AndroidIncludeDebugSymbols)' != 'true'"
Condition="'$(AndroidLinkMode)' != 'None' AND '$(AndroidIncludeDebugSymbols)' != 'true' AND '$(AndroidStripILAfterAOT)' != 'true'"
ShrunkFrameworkAssemblies="@(_ShrunkAssemblies)" />

<MakeDir Directories="$(MonoAndroidIntermediateAssemblyDir)shrunk" />
Expand Down
19 changes: 19 additions & 0 deletions tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,5 +1075,24 @@ public void SupportDesugaringStaticInterfaceMethods ()
);
}

[Test]
public void EnableAndroidStripILAfterAOT ()
Copy link
Member

Choose a reason for hiding this comment

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

Possibly silly question, but is there any way to test that $(AndroidStripILAfterAOT)=True actually does something? It could be a no-op and this test would still succeed!

Perhaps we should crack open the linked "app.dll" assembly and verify that e.g. MainActivity.OnCreate() has no body?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we should do something like this test:
https://github.com/xamarin/xamarin-android/blob/5f38d099fc39fd270dca7f0d5ab57ff163945988/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs#L293-L304

And then we could assert a method has 0 instructions?

Copy link
Member

Choose a reason for hiding this comment

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

or a null method body, yes.

Copy link
Member

Choose a reason for hiding this comment

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

That would be great, if we could create a test like that.

Copy link
Member

Choose a reason for hiding this comment

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

@jonathanpeppers: EnableAndroidStripILAfterAOT() should also test the "profiled AOT/don't AOT everything" case.

{
var proj = new XamarinAndroidApplicationProject {
IsRelease = true,
EnableDefaultItems = true,
};
proj.SetProperty("AndroidStripILAfterAOT", "true");

var builder = CreateApkBuilder ();
Assert.IsTrue (builder.Build (proj), "`dotnet build` should succeed");
RunProjectAndAssert (proj, builder);

WaitForPermissionActivity (Path.Combine (Root, builder.ProjectDirectory, "permission-logcat.log"));
bool didLaunch = WaitForActivityToStart (proj.PackageName, "MainActivity",
Path.Combine (Root, builder.ProjectDirectory, "logcat.log"), 30);
Assert.IsTrue(didLaunch, "Activity should have started.");
}

}
}