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

[XABT] Make assembly compression incremental. #9704

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Jan 23, 2025

If using $(AndroidEnableAssemblyCompression) today, we do not compress assemblies incrementally. That is, we always compress every assembly even if they haven't changed since the previous build.

Update the CompressAssemblies task to check if the input is newer than the output, and skip recompressing if the input has not changed.

This is most visible with the following test case:

dotnet new android
dotnet build -p:EmbedAssembliesIntoApk=true -p:AndroidIncludeDebugSymbols=false
Scenario (CompressAssemblies tasks) main This PR
Full 44.26 s 44.2 s
NoChanges not run 34 ms
ChangeResource 27.8 s 9 ms
AddResource 25.81 s 25 ms
ChangeCSharp 25.87 s 30 ms
ChangeCSharpJLO 27.04 s 23.36 s

ChangeCSharpJLO should be similarly reduced, however something earlier in the build process is causing Mono.Android.dll to get touched so it is getting recompressed. We'll leave that as an investigation for another day.

/// <summary>
/// Returns true if the input file is newer than the output file.
/// </summary>
public static bool IsFileOutOfDate (string input, string output)
Copy link
Member

@jonathanpeppers jonathanpeppers Jan 24, 2025

Choose a reason for hiding this comment

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

Is this logic in a separate target? We could use MSBuild's Inputs and Outputs to do this instead?

And then could test it with this helper method:

b.Output.AssertTargetIsPartiallyBuilt ("_CompileResources");

Copy link
Contributor Author

@jpobst jpobst Jan 24, 2025

Choose a reason for hiding this comment

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

I initially did it this way, but I didn't like that it calls the target X00 individual times. This flooded the binlog making it a lot harder to read.

From reading the docs, I had thought it would call the target once, providing an ItemGroup of inputs that needed to be updated, but that was not the case.

The other downside is that it needed 3 targets to do it this way:

  • Target to set up the Input ItemGroup with Destination metadata
  • The incremental target
  • Target to figure out which compressions were successful so that the APK building task knew whether to use the compressed file or the uncompressed file

Copy link
Member

Choose a reason for hiding this comment

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

MSBuild is definitely arcane, but you can make a target build partially and not "batch". <LinkAssembliesNoShrink/> is an example that runs the task once:

  • <Target Name="_LinkAssembliesNoShrink"
    DependsOnTargets="_LinkAssembliesNoShrinkInputs"
    Condition="'$(AndroidLinkMode)' == 'None'"
    Inputs="@(ResolvedAssemblies);$(_AndroidBuildPropertiesCache)"
    Outputs="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(DestinationSubPath)')">
    <LinkAssembliesNoShrink
    ResolvedAssemblies="@(_AllResolvedAssemblies)"
    SourceFiles="@(ResolvedAssemblies)"
    DestinationFiles="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(DestinationSubPath)')"
    TargetName="$(TargetName)"
    AddKeepAlives="$(AndroidAddKeepAlives)"
    UseDesignerAssembly="$(AndroidUseDesignerAssembly)"
    Deterministic="$(Deterministic)"
    ReadSymbols="$(_AndroidLinkAssembliesReadSymbols)"
    />
    <ItemGroup>
    <FileWrites Include="$(MonoAndroidIntermediateAssemblyDir)**" />
    </ItemGroup>
    </Target>

If it's easier to write C# code that is ok, if it has enough logging to know files changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, _LinkAssembliesNoShrink does appear to do exactly what I wanted to do:

image

Here's what I was using and I don't understand why it works differently:

<Target Name="_CompressAssemblies"
    DependsOnTargets="_CollectAssembliesToCompress"
    Inputs="@(_AssembliesToCompress)"
    Outputs="@(_AssembliesToCompress->%(DestinationPath))"
    Condition="'$(EmbedAssembliesIntoApk)' == 'True'">

    <CompressAssembly
        AssembliesToCompress="@(_AssembliesToCompress)">
      <Output TaskParameter="CompressionFailedAssemblies" ItemName="_CompressionFailedAssemblies" />
    </CompressAssembly>
</Target>

Copy link
Member

Choose a reason for hiding this comment

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

I would normally put single quote around: Outputs="@(_AssembliesToCompress->'%(DestinationPath)')"

Other than that, maybe it's the <Output/> that makes it behave differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, I guess the single quote was important. Without it I get:

image

Adding it makes it work as expected:

image

Thanks for the help!

@jpobst jpobst force-pushed the dev/jpobst/incremental-compression branch from d217bf1 to 679a43e Compare January 27, 2025 22:48
@jpobst jpobst force-pushed the dev/jpobst/incremental-compression branch from 679a43e to c097ec2 Compare January 28, 2025 18:01
@jpobst jpobst marked this pull request as ready for review January 28, 2025 22:50
@jpobst jpobst requested a review from grendello as a code owner January 28, 2025 22:50
case CompressionResult.EncodingFailed:
log.LogMessage ($"Failed to compress {sourceAssembly}");
break;
case CompressionResult.InputTooBig:
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewing the full code file this does not seem to be enabled anymore? The code which raised this issue is commented out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why that error condition is disabled in the existing code, but I think it's fine to ensure we log it properly if it ever gets reenabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants