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

[.NET 8] perf regression in <LinkAssembliesNoShrink/> #7684

Closed
jonathanpeppers opened this issue Jan 10, 2023 · 5 comments · Fixed by #7686
Closed

[.NET 8] perf regression in <LinkAssembliesNoShrink/> #7684

jonathanpeppers opened this issue Jan 10, 2023 · 5 comments · Fixed by #7686
Assignees
@dellis1972 @jonathanpeppers
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Milestone
.NET 8

Comments

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Jan 10, 2023 •
edited
Loading

Android application type

Android for .NET (net6.0-android, etc.)

Affected platform version

.NET 8 / main

Description

In .NET 7, in an old log I see:

image

This was an example of a .xaml change in a dotnet new maui project.

Now I see:

image

dotnet trace shows most of the time is:

image

So I think we might need to rework parts of dc3ccf2, maybe it can skip some things on incremental builds?

Less log messages would also help:

image

Steps to Reproduce

  1. dotnet new maui
  2. build
  3. change .xaml
  4. build again

Did you find any workaround?

Set $(AndroidUseDesignerAssembly) to false, would skip some of these steps.

Relevant log output

msbuild.zip
@jonathanpeppers jonathanpeppers added the Area: App+Library Build Issues when building Library projects or Application projects. label Jan 10, 2023
@jonathanpeppers jonathanpeppers added this to the .NET 8 milestone Jan 10, 2023
@ghost ghost added the needs-triage Issues that need to be assigned. label Jan 10, 2023
@dellis1972
Copy link
Contributor

This step is converting old Resource.designer.cs code over to the new system. So once Maui has switched over to using the newer system it should be better as it will have less work to do.

@dellis1972 dellis1972 removed the needs-triage Issues that need to be assigned. label Jan 10, 2023
@jonathanpeppers
Copy link
Member Author

Yeah, we can keep an eye and test again when we have a new MAUI.

What made me think something else is going on, is this target builds partially:

Target Name=_LinkAssembliesNoShrink Project=bar.csproj
Building target "_LinkAssembliesNoShrink" partially, because some output files are out of date with respect to their input files.
[ResolvedAssemblies: Input=obj\Debug\net8.0-android\android-arm64\bar.dll, Output=obj\Debug\net8.0-android\android-arm64\android\assets\bar.dll] Input file is newer than output file.

So in theory it should only be looking at bar.dll in this example?

@dellis1972
Copy link
Contributor

It is buiding partially. but the Task in the target is not using any of the "partial" calculated ItemGroups

<LinkAssembliesNoShrink
      ResolvedAssemblies="@(_AllResolvedAssemblies)"
      SourceFiles="@(ResolvedAssemblies)"
      DestinationFiles="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(DestinationSubPath)')"
      TargetName="$(TargetName)"
      AddKeepAlives="$(AndroidAddKeepAlives)"
      UseDesignerAssembly="$(AndroidUseDesignerAssembly)"
      Deterministic="$(Deterministic)"
      UsingAndroidNETSdk="$(UsingAndroidNETSdk)"
  />

Its using @(_AllResolvedAssemblies) , so all the items will be processed.

@jonathanpeppers
Copy link
Member Author

Previously @(_AllResolvedAssemblies) was only used for search paths on the DirectoryAssemblyResolver -- it's possible Mono.Cecil would need to go load other assemblies.

The "partial" part is:

  SourceFiles="@(ResolvedAssemblies)"
  DestinationFiles="@(ResolvedAssemblies->'$(MonoAndroidIntermediateAssemblyDir)%(DestinationSubPath)')"

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jan 11, 2023
@jonathanpeppers
[Xamarin.Android.Build.Tasks] lazily populate Resource lookup
30501c1 
Fixes: dotnet#7684

Comparing .NET 7 to main, I noticed:

    .NET 7
    Task LinkAssembliesNoShrink 4ms
    main/.NET 8
    Task LinkAssembliesNoShrink 101ms

Under `dotnet trace` a lot of the time was spent in:

    94.74ms MonoDroid.Tuner.FixLegacyResourceDesignerStep.LoadDesigner()

Reviewing the code, I think we can "lazily" call the `LoadDesigner()`
method. It creates a `Dictionary` that isn't used until the
`FixBody()` method.

I also updated one log message to only log duplicates, as it was
logging hundreds of lines:

    if (output.ContainsKey (key)) {
        LogMessage ($"          Found duplicate {key}");
    } else {
        output.Add (key, property.GetMethod);
    }

Which also showed up in `dotnet trace`:

    25.58ms Microsoft.Android.Build.Tasks.MSBuildExtensions.LogDebugMessage()

With these changes, I instead get:

    Task LinkAssembliesNoShrink 5ms

Which is probably ~the same performance as before or plenty good enough!
@jonathanpeppers jonathanpeppers mentioned this issue Jan 11, 2023
Merged
@jonathanpeppers
Copy link
Member Author

If I'm reading the code right, I think this will fix this: #7686

But let's see if anything breaks?

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jan 12, 2023
@jonathanpeppers
[Xamarin.Android.Build.Tasks] lazily populate Resource lookup
6e09a65 
Fixes: dotnet#7684

Comparing .NET 7 to main, I noticed:

    .NET 7
    Task LinkAssembliesNoShrink 4ms
    main/.NET 8
    Task LinkAssembliesNoShrink 101ms

Under `dotnet trace` a lot of the time was spent in:

    94.74ms MonoDroid.Tuner.FixLegacyResourceDesignerStep.LoadDesigner()

Reviewing the code, I think we can "lazily" call the `LoadDesigner()`
method. Now it delays until the `ProcessAssemblyDesigner()` method. I
had to reorder logic slightly to do this.

I also updated one log message to only log duplicates, as it was
logging hundreds of lines:

    if (output.ContainsKey (key)) {
        LogMessage ($"          Found duplicate {key}");
    } else {
        output.Add (key, property.GetMethod);
    }

Which also showed up in `dotnet trace`:

    25.58ms Microsoft.Android.Build.Tasks.MSBuildExtensions.LogDebugMessage()

With these changes, I instead get:

    Task LinkAssembliesNoShrink 5ms

Which is probably ~the same performance as before or plenty good enough!

Lastly, I also removed some other code & members in
`FixLegacyResourceDesignerStep` that Visual Studio showed me was
unused.
dellis1972 pushed a commit that referenced this issue Jan 17, 2023
@jonathanpeppers
[Xamarin.Android.Build.Tasks] lazily populate Resource lookup (#7686)
899d781 
Fixes: #7684

Comparing .NET 7 to main, I noticed:

    .NET 7
    Task LinkAssembliesNoShrink 4ms
    main/.NET 8
    Task LinkAssembliesNoShrink 101ms

Under `dotnet trace` a lot of the time was spent in:

    94.74ms MonoDroid.Tuner.FixLegacyResourceDesignerStep.LoadDesigner()

Reviewing the code, I think we can "lazily" call the `LoadDesigner()`
method. Now it delays until the `ProcessAssemblyDesigner()` method. I
had to reorder logic slightly to do this.

I also updated one log message to only log duplicates, as it was
logging hundreds of lines:

    if (output.ContainsKey (key)) {
        LogMessage ($"          Found duplicate {key}");
    } else {
        output.Add (key, property.GetMethod);
    }

Which also showed up in `dotnet trace`:

    25.58ms Microsoft.Android.Build.Tasks.MSBuildExtensions.LogDebugMessage()

With these changes, I instead get:

    Task LinkAssembliesNoShrink 5ms

Which is probably ~the same performance as before or plenty good enough!

Lastly, I also removed some other code & members in
`FixLegacyResourceDesignerStep` that Visual Studio showed me was
unused.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dellis1972 dellis1972

@jonathanpeppers jonathanpeppers

Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Projects
None yet
Milestone
.NET 8
Development

Successfully merging a pull request may close this issue.

[Xamarin.Android.Build.Tasks] lazily populate Resource lookup jonathanpeppers/xamarin-android
2 participants
@dellis1972 @jonathanpeppers