Skip to content

Commit

Permalink
[One .NET] <LinkAssembliesNoShrink/> can skip the main assembly (#6328)
Browse files Browse the repository at this point in the history 
Context: #6356
Context: #6357

We found an incremental build in a `dotnet new android` app with C#
code changes has this duration:

	100 ms  LinkAssembliesNoShrink                     1 calls

It turns out we don't actually need to run `<LinkAssembliesNoShrink/>`
against the "main" assembly in .NET 6.  After passing in
`$(TargetName)` and simply copying the file, this time improved to:

	  9 ms  LinkAssembliesNoShrink                     1 calls

This change should improve incremental C# changes by ~91ms in
`dotnet new android` projects.

In "Xammie" Xamarin.Android, it is possible for an application project
to reference a class library with a higher `$(TargetFrameworkVersion)`;
see [warning XA0105 docs][0].

We cannot apply this optimization for Xamarin.Android.

However, in thinking about and revisiting this code, we've made two
determinations for future work:

  * XA0105 being a warning is a terrible idea, as it can allow apps
    to crash via `TypeLoadException`.

    See #6356.

  * .NET 6 builds don't emit [XA2000 warnings][1] when a referenced
    assembly references `AppDomain.CreateDomain()`.  This isn't
    desirable, as `AppDomain.CreateDomain()` is still part of the
    .NET Standard 2.0 API contract, and thus could still be used,
    even though it won't work.  *Something* should warn if
    `AppDomain.CreateDomain()` is referenced, to reduce the need for
    manual app testing.

    See #6357.

[0]: https://github.com/xamarin/xamarin-android/blob/af60da7f13e613faa8ded5ef734e8c6d91445acc/Documentation/guides/messages/xa0105.md
[1]: https://github.com/xamarin/xamarin-android/blob/03d4e40d310255f80cc5ffb6475f03a02a561bc5/Documentation/guides/messages/xa2000.md
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Oct 1, 2021
1 parent 03d4e40 commit d7c0578
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 62 deletions.
42 changes: 15 additions & 27 deletions src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs
View file
Expand Up @@ -52,7 +52,7 @@ bool CheckShouldProcessAssembly (AssemblyDefinition assembly)
return false;

#if !NET5_LINKER
CheckAppDomainUsageUnconditional (assembly, (string msg) => Context.LogMessage (MessageImportance.High, msg));
CheckAppDomainUsage (assembly, (string msg) => Context.LogMessage (MessageImportance.High, msg));
#endif // !NET5_LINKER

return assembly.MainModule.HasTypeReference ("Java.Lang.Object");
Expand Down Expand Up @@ -86,22 +86,24 @@ protected override void ProcessAssembly (AssemblyDefinition assembly)
if (!CheckShouldProcessAssembly (assembly))
return;

if (FixAbstractMethodsUnconditional (assembly)) {
if (FixAbstractMethods (assembly)) {
Context.SafeReadSymbols (assembly);
UpdateAssemblyAction (assembly);
MarkAbstractMethodErrorType ();
}
}

internal void CheckAppDomainUsage (AssemblyDefinition assembly, Action<string> warn)
internal bool FixAbstractMethods (AssemblyDefinition assembly)
{
if (IsProductOrSdkAssembly (assembly))
return;

CheckAppDomainUsageUnconditional (assembly, warn);
bool changed = false;
foreach (var type in assembly.MainModule.Types) {
if (MightNeedFix (type))
changed |= FixAbstractMethods (type);
}
return changed;
}

void CheckAppDomainUsageUnconditional (AssemblyDefinition assembly, Action<string> warn)
internal void CheckAppDomainUsage (AssemblyDefinition assembly, Action<string> warn)
{
if (!assembly.MainModule.HasTypeReference ("System.AppDomain"))
return;
Expand All @@ -113,27 +115,13 @@ void CheckAppDomainUsageUnconditional (AssemblyDefinition assembly, Action<strin
}
}
}

internal bool FixAbstractMethods (AssemblyDefinition assembly)
{
return !IsProductOrSdkAssembly (assembly) && FixAbstractMethodsUnconditional (assembly);
}

bool FixAbstractMethodsUnconditional (AssemblyDefinition assembly)
{
bool changed = false;
foreach (var type in assembly.MainModule.Types) {
if (MightNeedFix (type))
changed |= FixAbstractMethods (type);
}
return changed;
}
#endif // !NET5_LINKER

bool IsProductOrSdkAssembly (AssemblyDefinition assembly)
{
return Profile.IsSdkAssembly (assembly) || Profile.IsProductAssembly (assembly);
}
bool IsProductOrSdkAssembly (AssemblyDefinition assembly) =>
IsProductOrSdkAssembly (assembly.Name.Name);

public bool IsProductOrSdkAssembly (string assemblyName) =>
Profile.IsSdkAssembly (assemblyName) || Profile.IsProductAssembly (assemblyName);

bool MightNeedFix (TypeDefinition type)
{
Expand Down
73 changes: 46 additions & 27 deletions src/Xamarin.Android.Build.Tasks/Tasks/LinkAssembliesNoShrink.cs
View file
@@ -1,11 +1,10 @@
#nullable enable
using Java.Interop.Tools.Cecil;
using Microsoft.Build.Framework;
using Microsoft.Build.Utilities;
using Mono.Cecil;
using System;
using System.IO;

using MTProfile = Mono.Tuner.Profile;
using Microsoft.Android.Build.Tasks;

namespace Xamarin.Android.Tasks
Expand All @@ -21,13 +20,21 @@ public class LinkAssembliesNoShrink : AndroidTask
/// These are used so we have the full list of SearchDirectories
/// </summary>
[Required]
public ITaskItem [] ResolvedAssemblies { get; set; }
public ITaskItem [] ResolvedAssemblies { get; set; } = Array.Empty<ITaskItem> ();

[Required]
public ITaskItem [] SourceFiles { get; set; } = Array.Empty<ITaskItem> ();

[Required]
public ITaskItem [] SourceFiles { get; set; }
public ITaskItem [] DestinationFiles { get; set; } = Array.Empty<ITaskItem> ();

/// <summary>
/// $(TargetName) would be "AndroidApp1" with no extension
/// </summary>
[Required]
public ITaskItem [] DestinationFiles { get; set; }
public string TargetName { get; set; } = "";

public bool UsingAndroidNETSdk { get; set; }

public bool AddKeepAlives { get; set; }

Expand All @@ -45,61 +52,73 @@ public override bool RunTask ()
DeterministicMvid = Deterministic,
};

var hasSystemPrivateCorelib = false;
using (var resolver = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: true, loadReaderParameters: readerParameters)) {
// Add SearchDirectories with ResolvedAssemblies
foreach (var assembly in ResolvedAssemblies) {
var path = Path.GetFullPath (Path.GetDirectoryName (assembly.ItemSpec));
if (Path.GetFileName (assembly.ItemSpec).Equals ("System.Private.CoreLib.dll", StringComparison.OrdinalIgnoreCase))
hasSystemPrivateCorelib = true;

if (!resolver.SearchDirectories.Contains (path))
resolver.SearchDirectories.Add (path);
}

// Set up the FixAbstractMethodsStep
var step1 = new FixAbstractMethodsStep (resolver, new TypeDefinitionCache (), Log);
// Set up the AddKeepAlivesStep
var step2 = new AddKeepAlivesStep (resolver, new TypeDefinitionCache (), Log, hasSystemPrivateCorelib);
// Set up the FixAbstractMethodsStep and AddKeepAlivesStep
var cache = new TypeDefinitionCache ();
var fixAbstractMethodsStep = new FixAbstractMethodsStep (resolver, cache, Log);
var addKeepAliveStep = new AddKeepAlivesStep (resolver, cache, Log, UsingAndroidNETSdk);
for (int i = 0; i < SourceFiles.Length; i++) {
var source = SourceFiles [i];
var destination = DestinationFiles [i];
AssemblyDefinition assemblyDefinition = null;

var assemblyName = Path.GetFileNameWithoutExtension (source.ItemSpec);
if (!MTProfile.IsSdkAssembly (assemblyName) && !MTProfile.IsProductAssembly (assemblyName)) {

// In .NET 6+, we can skip the main assembly
if (UsingAndroidNETSdk && !AddKeepAlives && assemblyName == TargetName) {
CopyIfChanged (source, destination);
continue;
}
if (fixAbstractMethodsStep.IsProductOrSdkAssembly (assemblyName)) {
CopyIfChanged (source, destination);
continue;
}

// Check AppDomain usage on any non-Product or Sdk assembly
AssemblyDefinition? assemblyDefinition = null;
if (!UsingAndroidNETSdk) {
assemblyDefinition = resolver.GetAssembly (source.ItemSpec);
step1.CheckAppDomainUsage (assemblyDefinition, (string msg) => Log.LogMessageFromText (msg, MessageImportance.High));
fixAbstractMethodsStep.CheckAppDomainUsage (assemblyDefinition, (string msg) => Log.LogMessageFromText (msg, MessageImportance.High));
}

// Only run the step on "MonoAndroid" assemblies
if (MonoAndroidHelper.IsMonoAndroidAssembly (source) && !MonoAndroidHelper.IsSharedRuntimeAssembly (source.ItemSpec)) {
if (assemblyDefinition == null)
assemblyDefinition = resolver.GetAssembly (source.ItemSpec);

if (step1.FixAbstractMethods (assemblyDefinition) ||
(AddKeepAlives && step2.AddKeepAlives (assemblyDefinition))) {
if (fixAbstractMethodsStep.FixAbstractMethods (assemblyDefinition) ||
(AddKeepAlives && addKeepAliveStep.AddKeepAlives (assemblyDefinition))) {
Log.LogDebugMessage ($"Saving modified assembly: {destination.ItemSpec}");
writerParameters.WriteSymbols = assemblyDefinition.MainModule.HasSymbols;
assemblyDefinition.Write (destination.ItemSpec, writerParameters);
continue;
}
}

if (MonoAndroidHelper.CopyAssemblyAndSymbols (source.ItemSpec, destination.ItemSpec)) {
Log.LogDebugMessage ($"Copied: {destination.ItemSpec}");
} else {
Log.LogDebugMessage ($"Skipped unchanged file: {destination.ItemSpec}");

// NOTE: We still need to update the timestamp on this file, or this target would run again
File.SetLastWriteTimeUtc (destination.ItemSpec, DateTime.UtcNow);
}
CopyIfChanged (source, destination);
}
}

return !Log.HasLoggedErrors;
}

void CopyIfChanged (ITaskItem source, ITaskItem destination)
{
if (MonoAndroidHelper.CopyAssemblyAndSymbols (source.ItemSpec, destination.ItemSpec)) {
Log.LogDebugMessage ($"Copied: {destination.ItemSpec}");
} else {
Log.LogDebugMessage ($"Skipped unchanged file: {destination.ItemSpec}");

// NOTE: We still need to update the timestamp on this file, or this target would run again
File.SetLastWriteTimeUtc (destination.ItemSpec, DateTime.UtcNow);
}
}

class FixAbstractMethodsStep : MonoDroid.Tuner.FixAbstractMethodsStep
{
readonly DirectoryAssemblyResolver resolver;
Expand Down
34 changes: 26 additions & 8 deletions src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Tasks/LinkerTests.cs
View file
Expand Up @@ -179,16 +179,34 @@ public void PreserveCustomHttpClientHandlers ()
}

[Test]
[Category ("DotNetIgnore")] // n/a on .NET 5+
public void WarnAboutAppDomains ([Values (true, false)] bool isRelease)
{
var proj = new XamarinAndroidApplicationProject () { IsRelease = isRelease };
proj.MainActivity = proj.DefaultMainActivity.Replace ("base.OnCreate (bundle);", "base.OnCreate (bundle);\nvar appDomain = System.AppDomain.CreateDomain (\"myDomain\");");
using (var b = CreateApkBuilder ()) {
Assert.IsTrue (b.Build (proj), "Build should have succeeded.");
Assert.IsTrue (StringAssertEx.ContainsText (b.LastBuildOutput, "2 Warning(s)"), "MSBuild should count 2 warnings.");
Assert.IsTrue (StringAssertEx.ContainsText (b.LastBuildOutput, "warning CS0618: 'AppDomain.CreateDomain(string)' is obsolete: 'AppDomain.CreateDomain will no longer be supported in .NET 5 and later."), "Should warn CS0618 about creating AppDomain.");
Assert.IsTrue (StringAssertEx.ContainsText (b.LastBuildOutput, "warning XA2000: Use of AppDomain.CreateDomain()"), "Should warn XA2000 about creating AppDomain.");
var path = Path.Combine (Root, "temp", TestName);
var lib = new XamarinAndroidLibraryProject {
IsRelease = isRelease,
ProjectName = "Library",
Sources = {
new BuildItem.Source ("Foo.cs") {
TextContent = () => "class Foo { System.AppDomain Bar => System.AppDomain.CreateDomain (\"myDomain\"); }",
}
}
};

var app = new XamarinAndroidApplicationProject { IsRelease = isRelease };
app.AddReference (lib);
using var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName));
Assert.IsTrue (libBuilder.Build (lib), "library build should have succeeded.");
// AppDomain.CreateDomain() is [Obsolete]
Assert.IsTrue (StringAssertEx.ContainsText (libBuilder.LastBuildOutput, "1 Warning(s)"), "MSBuild should count 1 warnings.");

using var appBuilder = CreateApkBuilder (Path.Combine (path, app.ProjectName));
Assert.IsTrue (appBuilder.Build (app), "app build should have succeeded.");

if (Builder.UseDotNet) {
appBuilder.AssertHasNoWarnings ();
} else {
Assert.IsTrue (StringAssertEx.ContainsText (appBuilder.LastBuildOutput, "1 Warning(s)"), "MSBuild should count 1 warnings.");
Assert.IsTrue (StringAssertEx.ContainsText (appBuilder.LastBuildOutput, "warning XA2000: Use of AppDomain.CreateDomain()"), "Should warn XA2000 about creating AppDomain.");
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets
View file
Expand Up @@ -1373,8 +1373,10 @@ because xbuild doesn't support framework reference assemblies.
ResolvedAssemblies="@(_AllResolvedAssemblies)"
SourceFiles="@(ResolvedAssemblies)"
DestinationFiles="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(DestinationSubPath)')"
TargetName="$(TargetName)"
AddKeepAlives="$(AndroidAddKeepAlives)"
Deterministic="$(Deterministic)"
UsingAndroidNETSdk="$(UsingAndroidNETSdk)"
/>
<ItemGroup>
<FileWrites Include="$(MonoAndroidIntermediateAssemblyDir)**" />
Expand Down

0 comments on commit d7c0578

Please sign in to comment.