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

[Xamarin.Android.Build.Tasks] lazily populate Resource lookup #7686

Merged

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jan 11, 2023

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.

Fixes: xamarin#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.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review January 12, 2023 23:11
@jonathanpeppers
Copy link
Member Author

I think it's going to work this time, my first attempt triggered the failure:

ApplicationRunsWithoutDebugger(False,False,False)
Activity should have started.
Expected: True
But was:  False

So we should have decent test coverage on this.

@dellis1972
Copy link
Contributor

This might have been an ovesight on my part, but I'm not sure we have a test which specifically tests this senario on device.
i.e an app which does use the new designer system with a library that does not. We might need to add one or modify an existing one to do that.

Problem is the test would need to be DotNet only since the new system is not supported.

@jonathanpeppers
Copy link
Member Author

When I was debugging ApplicationRunsWithoutDebugger, I found it was actually testing this scenario (why I got a failure with my first attempt 30501c1 before when I broke something):

[Test]
[Category ("Node-3")]
public void ApplicationRunsWithoutDebugger ([Values (false, true)] bool isRelease, [Values (false, true)] bool extractNativeLibs, [Values (false, true)] bool useEmbeddedDex)
{
AssertHasDevices ();
SwitchUser ();
var proj = new XamarinFormsAndroidApplicationProject () {
IsRelease = isRelease,
};

Because this app is Xamarin.Forms (which uses Resource.designer.cs in a class library), I was getting an InvalidCastException on startup when this was wrong. It had an incorrect integer for a resource id, which caused the exception at startup.

@dellis1972 dellis1972 merged commit 899d781 into xamarin:main Jan 17, 2023
@jonathanpeppers jonathanpeppers deleted the FixLegacyResourceDesignerStep branch January 17, 2023 16:05
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 18, 2023
* main:
  [Mono.Android] Android.Telecom.InCallService.SetAudioRoute() + enum (xamarin#7711)
  [Mono.Android] Fix some incorrect enums. (xamarin#7670)
  [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer namespace (xamarin#7681)
  LEGO: Merge pull request 7713
  [Xamarin.Android.Build.Tasks] lazily populate Resource lookup (xamarin#7686)
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 19, 2023
* main:
  [ci] Pass token when building Designer tests (xamarin#7715)
  [Mono.Android] Android.Telecom.InCallService.SetAudioRoute() + enum (xamarin#7711)
  [Mono.Android] Fix some incorrect enums. (xamarin#7670)
  [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer namespace (xamarin#7681)
  LEGO: Merge pull request 7713
  [Xamarin.Android.Build.Tasks] lazily populate Resource lookup (xamarin#7686)
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 26, 2023
* main: (32 commits)
  [monodroid] Replace `exit()` with `abort()` in native code (xamarin#7734)
  Bump to xamarin/java.interop@8a1ae57 (xamarin#7738)
  [build] bump `$(AndroidNet7Version)` (xamarin#7737)
  Bump to xamarin/java.interop@1366d99 (xamarin#7718)
  [Xamarin.Android.Build.Tasks] fix AndroidGenerateResourceDesigner (xamarin#7721)
  Bump to xamarin/monodroid@50faac94 (xamarin#7725)
  Revert "[Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (xamarin#7642)" (xamarin#7726)
  [marshal methods] Properly support arrays of arrays (xamarin#7707)
  Bump to dotnet/installer@9962c6a 8.0.100-alpha.1.23063.11 (xamarin#7677)
  [Actions] Add action to bump the hash used for the unified pipeline (xamarin#7712)
  Bump to xamarin/xamarin-android-tools@099fd95 (xamarin#7709)
  [ci] Move build stages into yaml templates (xamarin#7553)
  [Xamarin.Android.Build.Tasks] fix NRE in `<GenerateResourceCaseMap/>` (xamarin#7716)
  [ci] Pass token when building Designer tests (xamarin#7715)
  [Mono.Android] Android.Telecom.InCallService.SetAudioRoute() + enum (xamarin#7711)
  [Mono.Android] Fix some incorrect enums. (xamarin#7670)
  [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer namespace (xamarin#7681)
  LEGO: Merge pull request 7713
  [Xamarin.Android.Build.Tasks] lazily populate Resource lookup (xamarin#7686)
  [Xamarin.Android.Build.Tasks] skip XA1034 logic in some cases (xamarin#7680)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
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 this pull request may close these issues.

[.NET 8] perf regression in <LinkAssembliesNoShrink/>
2 participants