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] FixAbstractMethodsStep performance #8650

Merged
merged 1 commit into from
Feb 7, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Jan 18, 2024

Context: #8421

Working a bit on build performance, I tested:

  • dotnet new maui

  • dotnet build -bl

The .binlog shows:

LinkAssembliesNoShrink 3.797s

Attaching dotnet-trace as mentioned on:

https://github.com/xamarin/xamarin-android/blob/2f192386e8072f8e0ecaf0de2fe48654f3ade423/Documentation/guides/tracing.md#how-to-dotnet-trace-our-build

I see time broken down such as:

FixAbstractMethods: 37%
AssemblyDefinition.Write: 27%
ProcessAssemblyDesigner: 20%
CopyIfChanged: 13%
DirectoryAssemblyResolver.GetAssembly: 4.4%

This made me focus in on FixAbstractMethodsStep and make the following changes:

  • All calls for TypeReference.Resolve() and MethodReference.Resolve() should go through the TypeDefinitionCache to avoid repeated lookups.

  • IsInOverrides() can compare the MethodReference.Name before calling Resolve(). It could resolve many unnecessary methods otherwise.

After these changes, I instead see from dotnet-trace:

--1.45s (3.7%)    xamarin.android.build.tasks!MonoDroid.Tuner.FixAbstractMethodsStep.FixAbstractMethods()
++949.70ms (2.5%) xamarin.android.build.tasks!MonoDroid.Tuner.FixAbstractMethodsStep.FixAbstractMethods()

Time is now broken down differently, such as:

AssemblyDefinition.Write: 31%
FixAbstractMethods: 28%
ProcessAssemblyDesigner: 23%
CopyIfChanged: 12%
DirectoryAssemblyResolver.GetAssembly: 4.8%

In an overall .binlog (without dotnet-trace attached):

--LinkAssembliesNoShrink 3.797s
++LinkAssembliesNoShrink 3.105s

This saved ~700ms on initial build of a new MAUI project.

Context: dotnet#8421

Working a bit on build performance, I tested:

* `dotnet new maui`

* `dotnet build -bl`

The `.binlog` shows:

    LinkAssembliesNoShrink 3.797s

Attaching `dotnet-trace` as mentioned on:

https://github.com/xamarin/xamarin-android/blob/2f192386e8072f8e0ecaf0de2fe48654f3ade423/Documentation/guides/tracing.md#how-to-dotnet-trace-our-build

I see time broken down such as:

    FixAbstractMethods: 37%
    AssemblyDefinition.Write: 27%
    ProcessAssemblyDesigner: 20%
    CopyIfChanged: 13%
    DirectoryAssemblyResolver.GetAssembly: 4.4%

This made me focus in on `FixAbstractMethodsStep` and make the following
changes:

* All calls for `TypeReference.Resolve()` and `MethodReference.Resolve()`
  should go through the `TypeDefinitionCache` to avoid repeated lookups.

* `IsInOverrides()` can compare the `MethodReference.Name` before
  calling `Resolve()`. It could resolve many unnecessary methods otherwise.

After these changes, I instead see from `dotnet-trace`:

```
--1.45s (3.7%)    xamarin.android.build.tasks!MonoDroid.Tuner.FixAbstractMethodsStep.FixAbstractMethods()
++949.70ms (2.5%) xamarin.android.build.tasks!MonoDroid.Tuner.FixAbstractMethodsStep.FixAbstractMethods()
```

Time is now broken down differently, such as:

    AssemblyDefinition.Write: 31%
    FixAbstractMethods: 28%
    ProcessAssemblyDesigner: 23%
    CopyIfChanged: 12%
    DirectoryAssemblyResolver.GetAssembly: 4.8%

In an overall `.binlog` (without `dotnet-trace` attached):

```
--LinkAssembliesNoShrink 3.797s
++LinkAssembliesNoShrink 3.105s
```

This saved ~700ms on initial build of a new MAUI project.
@jonathanpeppers
Copy link
Member Author

Ignore the MAUI lane for now, I think their build is broken:

C:\a\_work\3\s\maui\src\Core\src\Platform\Tizen\StackNavigationManager.cs(73,25): error CA1859: Change type of variable 'newPageStack' from 'System.Collections.Generic.IReadOnlyList<Microsoft.Maui.IView>' to 'System.Collections.Generic.List<Microsoft.Maui.IView>' for improved performance (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859) [C:\a\_work\3\s\maui\src\Core\src\Core.csproj::TargetFramework=net9.0-tizen]
C:\a\_work\3\s\maui\src\Core\src\Platform\Windows\StackNavigationManager.cs(65,25): error CA1859: Change type of variable 'newPageStack' from 'System.Collections.Generic.IReadOnlyList<Microsoft.Maui.IView>' to 'System.Collections.Generic.List<Microsoft.Maui.IView>' for improved performance (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859) [C:\a\_work\3\s\maui\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.20348]
C:\a\_work\3\s\maui\src\Core\src\Platform\Windows\StackNavigationManager.cs(65,25): error CA1859: Change type of variable 'newPageStack' from 'System.Collections.Generic.IReadOnlyList<Microsoft.Maui.IView>' to 'System.Collections.Generic.List<Microsoft.Maui.IView>' for improved performance (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859) [C:\a\_work\3\s\maui\src\Core\src\Core.csproj::TargetFramework=net9.0-windows10.0.19041]

@jonathanpeppers jonathanpeppers marked this pull request as ready for review January 19, 2024 00:42
@jonpryor jonpryor merged commit 526172b into dotnet:main Feb 7, 2024
45 of 47 checks passed
@jonathanpeppers jonathanpeppers deleted the FixAbstractMethodsStep branch February 7, 2024 17:37
grendello added a commit that referenced this pull request Feb 7, 2024
* main:
  [Xamarin.Android.Build.Tasks] `FixAbstractMethodsStep` performance (#8650)
grendello added a commit that referenced this pull request Feb 8, 2024
* main:
  Bump to xamarin/monodroid@848d1277b7 (#8691)
  [Xamarin.Android.Build.Tasks] `FixAbstractMethodsStep` performance (#8650)
grendello added a commit that referenced this pull request Feb 13, 2024
* main:
  [Xamarin.Android.Build.Tasks] remove `$(AndroidSupportedAbis)` from `build.props` (#8717)
  [Xamarin.Android.Build.Tasks] BannedApiAnalyzers for Resolve() (#8715)
  Bump to xamarin/Java.Interop/main@dfcbd670 (#8714)
  [monodroid] C++ tweaks and legacy code cleanup (#8638)
  Bump to xamarin/xamarin-android-tools/main@a698a33 (#8710)
  [readme] Add `d17-8` download links. (#8709)
  Bump external/Java.Interop from `07c7300` to `7f08b77` (#8702)
  Bump to xamarin/monodroid@848d1277b7 (#8691)
  [Xamarin.Android.Build.Tasks] `FixAbstractMethodsStep` performance (#8650)
  Bump to dotnet/installer@fb7b9a4b9e 9.0.100-preview.2.24106.6 (#8700)
grendello added a commit that referenced this pull request Feb 14, 2024
* main: (116 commits)
  [tmt] Update to work with current `libxamarin-app.so` (#8694)
  [Xamarin.Android.Build.Tasks] remove `$(AndroidSupportedAbis)` from `build.props` (#8717)
  [Xamarin.Android.Build.Tasks] BannedApiAnalyzers for Resolve() (#8715)
  Bump to xamarin/Java.Interop/main@dfcbd670 (#8714)
  [monodroid] C++ tweaks and legacy code cleanup (#8638)
  Bump to xamarin/xamarin-android-tools/main@a698a33 (#8710)
  [readme] Add `d17-8` download links. (#8709)
  Bump external/Java.Interop from `07c7300` to `7f08b77` (#8702)
  Bump to xamarin/monodroid@848d1277b7 (#8691)
  [Xamarin.Android.Build.Tasks] `FixAbstractMethodsStep` performance (#8650)
  Bump to dotnet/installer@fb7b9a4b9e 9.0.100-preview.2.24106.6 (#8700)
  [Mono.Android] Cache `Profiles/api-34.xml` contents (#8679)
  [monodroid] typemaps may need to load assemblies (#8625)
  Bump $(AndroidNetPreviousVersion) to 34.0.79 (#8693)
  Bump to xamarin/java.interop/main@07c73009 (#8681)
  Bump to dotnet/installer@1c496970b7 9.0.100-preview.2.24078.1 (#8685)
  [GetAndroidDependencies] Add Jdk dependency info (#8651)
  [xaprepare] Add support for newer SparkyLinux (#8684)
  Bump to dotnet/installer@5680e93cb2 9.0.100-preview.2.24073.12 (#8666)
  $(AndroidPackVersionSuffix)=preview.2; net9 is 34.99.0.preview.2 (#8678)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Mar 9, 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.

2 participants