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] delay ToJniName calls in ManifestDocument #7653

Merged

Conversation

jonathanpeppers
Copy link
Member

Reviewing dotnet trace output for a dotnet new maui app:

dotnet trace collect --format speedscope -- .\bin\Release\dotnet\dotnet.exe build -bl --no-restore foo.csproj

8.6% of the time in <GenerateJavaStubs/> is spent in:

93.81ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

Drilling down further, a lot of time is spent in:

26.91ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToJniName(...)
 4.03ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToCompatJniName(...)

It looks like we call this code for every Java.Lang.Object subclass:

var name        = JavaNativeTypeManager.ToJniName (t, cache).Replace ('/', '.');
var compatName  = JavaNativeTypeManager.ToCompatJniName (t, cache).Replace ('/', '.');
if (((string) app.Attribute (attName)) == compatName) {
    app.SetAttributeValue (attName, name);
}

It looks like this code was originally here to solve a problem with Android.App.Application classes that are missing an [Application] attribute:

https://github.com/xamarin/monodroid/commit/2a668b71f821ea0f625626439668c4ee31999526

To improve this:

  1. First check if we are a Android.App.Application, apply the above fixup and continue.

  2. Find out if we are other types of interesting subclasses, where a generator is needed. continue otherwise.

  3. Lastly, compute name & compatName. We are no longer doing this work for every type.

After these changes, I instead get:

72.84ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

This saves about ~21ms on incremental builds of dotnet new maui projects.

Reviewing `dotnet trace` output for a `dotnet new maui` app:

    dotnet trace collect --format speedscope -- .\bin\Release\dotnet\dotnet.exe build -bl --no-restore foo.csproj

8.6% of the time in `<GenerateJavaStubs/>` is spent in:

    93.81ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

Drilling down further, a lot of time is spent in:

    26.91ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToJniName(...)
     4.03ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToCompatJniName(...)

It looks like we call this code for *every* Java.Lang.Object subclass:

    var name        = JavaNativeTypeManager.ToJniName (t, cache).Replace ('/', '.');
    var compatName  = JavaNativeTypeManager.ToCompatJniName (t, cache).Replace ('/', '.');
    if (((string) app.Attribute (attName)) == compatName) {
        app.SetAttributeValue (attName, name);
    }

It looks like this code was originally here to solve a problem with
`Android.App.Application` classes that are missing an `[Application]`
attribute:

xamarin/monodroid@2a668b7

To improve this:

1. First check if we are a `Android.App.Application`, apply the above
   fixup and `continue`.

2. Find out if we are other types of interesting subclasses, where a
   `generator` is needed. `continue` otherwise.

3. Lastly, compute `name` & `compatName`. We are no longer doing this
   work for every type.

After these changes, I instead get:

    72.84ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

This saves about ~21ms on incremental builds of `dotnet new maui`
projects.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review December 22, 2022 14:51
var compatName = JavaNativeTypeManager.ToCompatJniName (t, cache).Replace ('/', '.');
if (((string) app.Attribute (attName)) == compatName) {
app.SetAttributeValue (attName, name);
string name, compatName;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of declaring w/o initializing, I think it would be better to "double declare" them, once within the following if block, and separately within the try block:

if (t.IsSubclassOf ("Android.App.Application", cache)) {
    var (name, compatName) = GetNames (t, cache);
    // …
}try {
    var (name, compatName) = GetNames (t, cache);
    // …
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I put (string name, string compatName) = GetNames (t, cache) so they types are more clear.

@jonathanpeppers jonathanpeppers merged commit fe18cc6 into dotnet:main Jan 4, 2023
@jonathanpeppers jonathanpeppers deleted the ManifestDocumentToJniName branch January 4, 2023 14:22
jonathanpeppers added a commit that referenced this pull request Jan 5, 2023
…nt (#7653)

Reviewing `dotnet trace` output for a `dotnet new maui` app:

    dotnet trace collect --format speedscope -- .\bin\Release\dotnet\dotnet.exe build -bl --no-restore foo.csproj

8.6% of the time in `<GenerateJavaStubs/>` is spent in:

    93.81ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

Drilling down further, a lot of time is spent in:

    26.91ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToJniName(...)
     4.03ms java.interop.tools.javacallablewrappers!Java.Interop.Tools.TypeNameMappings.JavaNativeTypeManager.ToCompatJniName(...)

It looks like we call this code for *every* Java.Lang.Object subclass:

    var name        = JavaNativeTypeManager.ToJniName (t, cache).Replace ('/', '.');
    var compatName  = JavaNativeTypeManager.ToCompatJniName (t, cache).Replace ('/', '.');
    if (((string) app.Attribute (attName)) == compatName) {
        app.SetAttributeValue (attName, name);
    }

It looks like this code was originally here to solve a problem with
`Android.App.Application` classes that are missing an `[Application]`
attribute:

xamarin/monodroid@2a668b7

To improve this:

1. First check if we are a `Android.App.Application`, apply the above
   fixup and `continue`.

2. Find out if we are other types of interesting subclasses, where a
   `generator` is needed. `continue` otherwise.

3. Lastly, compute `name` & `compatName`. We are no longer doing this
   work for every type.

After these changes, I instead get:

    72.84ms xamarin.android.build.tasks!Xamarin.Android.Tasks.ManifestDocument.Merge(...)

This saves about ~21ms on incremental builds of `dotnet new maui`
projects.
grendello added a commit to grendello/xamarin-android that referenced this pull request Jan 17, 2023
* main:
  [Xamarin.Android.Build.Tasks] skip XA1034 logic in some cases (dotnet#7680)
  [ci] Move OneLocBuild task to scheduled pipeline (dotnet#7679)
  [Mono.Android] ServerCertificateValidationCallback() and redirects (dotnet#7662)
  Bump to xamarin/Java.Interop/main@cf80deb7 (dotnet#7664)
  Localized file check-in by OneLocBuild (dotnet#7668)
  [api-merge] Correctly compute //method/@deprecated-since (dotnet#7645)
  [Xamarin.Android.Build.Tasks] _Microsoft.Android.Resource.Designer (dotnet#6427)
  [Xamarin.Android.Build.Tasks] downgrade d8/r8 `warning` messages to `info` (dotnet#7643)
  [Xamarin.Android.Build.Tasks] fix cases of missing `@(Reference)` (dotnet#7642)
  [Xamarin.Android.Build.Tasks] delay ToJniName calls in ManifestDocument (dotnet#7653)
  [Xamarin.Android.Build.Tasks] fast path for `<CheckClientHandlerType/>` (dotnet#7652)
@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.

3 participants