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

$(AndroidUseAapt2) runs ConvertResourcesCases a lot #2356

Closed
jonathanpeppers opened this issue Oct 30, 2018 · 3 comments · Fixed by #2367
Closed

$(AndroidUseAapt2) runs ConvertResourcesCases a lot #2356

jonathanpeppers opened this issue Oct 30, 2018 · 3 comments · Fixed by #2367
Assignees
@dellis1972 @jonathanpeppers
Labels
Area: Performance Issues with performance.
Milestone
d16-0

Comments

@jonathanpeppers
Copy link
Member

Steps to Reproduce

  1. Enable $(AndroidUseAapt2) in a Xamarin.Forms project
  2. You will see ConvertResourcesCases run ~9 times

See #2348, where I saw this firsthand. I think if we fixed this, $(AndroidUseAapt2) could possibly be enabled by default?

Expected Behavior

With $(AndroidUseAapt2), ConvertResourcesCases should take approximately the same amount of time as a build without $(AndroidUseAapt2).

Actual Behavior

$(AndroidUseAapt2) runs ConvertResourcesCases a bit too much.

Version Information

Reports of this in the 15.9 preview, and I have reproduced on master.

Log File

ConvertResourcesCases3.zip
@jonathanpeppers jonathanpeppers added the Area: Performance Issues with performance. label Oct 30, 2018
@dellis1972
Copy link
Contributor

When Aapt2 is enabled we have to generate a flata archive for each library resource (i.e everything in the lp directory. As part of that process we are calling ConvertResourcesCases to ensure that the library resources are fixed up BEFORE we put them in the flata. This happens on the very first build of the app, and unless a Clean is called will be a one off. aapt2 will then use those flata files in the link process later in the build.

Now we could make use of the new logic to Skip ConvertResourcesCases for those known libraries which we know do not need processing so help speed things up.

@jonathanpeppers
Copy link
Member Author

What I'm seeing from these logs, if I just pick one of the files ConvertResourcesCases is processing, this log shows up 9 times during the initial build:

  Processing: C:\Users\myuser\Desktop\Git\xamarin-android\tests\Xamarin.Forms-Performance-Integration\Droid\obj\Debug\90\lp\22\jl\res\values\values.xml   10/30/2018 4:28:18 PM > 1/1/0001 12:00:00 AM

So it would be ok for ConvertResourcesCases to run 9 times, if it wasn't running on the same files. But looking at ConvertResourcesCases, it seems like it's meant to run on all of them in one shot...

I'm wondering if ConvertResourcesCases could be moved to a new target and use DependsOnTargets? Then it could probably be setup to run once during the build? I will test and see if this breaks anything.

@jonathanpeppers jonathanpeppers added this to the d16-0 milestone Oct 31, 2018
@dellis1972
Copy link
Contributor

Ah right, yes. That is probably it. Aapt2 will need to run for each lp but the conversion should only run either across the whole lot or only for the res dir for that lp.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Nov 1, 2018
@jonathanpeppers
[Xamarin.Android.Build.Tasks] aapt2, move ConvertResourcesCases to a …
1f647ec 
…new target

Fixes: dotnet#2356

When `$(AndroidUseAapt2)=True`, if you look for a log entry of a file
that is processed:

    Processing: C:\Users\myuser\Desktop\Git\xamarin-android\tests\Xamarin.Forms-Performance-Integration\Droid\obj\Debug\90\lp\22\jl\res\values\values.xml   10/30/2018 4:28:18 PM > 1/1/0001 12:00:00 AM

You will see this repeated several times (9 in case of a Xamarin.Forms
project).

It turns out, since the `_CompileAndroidLibraryResources` target is
batched, it is called once per item of
`@(_LibraryResourceHashDirectories)`:

    <Target Name="_CompileAndroidLibraryResources" DependsOnTargets="_CollectLibraryResourceDirectories"
            Condition="'$(_AndroidUseAapt2)' == 'True'"
            Inputs="%(_LibraryResourceHashDirectories.FileFound)"
            Outputs="$(_AndroidLibraryFlatArchivesDirectory)%(_LibraryResourceHashDirectories.Hash).stamp">
    ...
    <ConvertResourcesCases ... />
    <Aapt2Compile ... />
    ...
    </Target>

It calls `ConvertResourcesCases` for each call, which is not ideal.
`ConvertResourcesCases` operates on the whole tree of resources, so it
really should just run once: *before* `<Aapt2Compile/>`.

I think the fix here is:

- Move `ConvertResourcesCases` to a new
  `_ConvertLibraryResourcesCases` target
- The new target's `Inputs` should be
  `@(_LibraryResourceHashDirectories.FileFound)` and *not* batched.
- The new target's `Outputs` should be a new stamp file following our
  new convention.
- The new stamp file should also be used as
  `AndroidConversionFlagFile`.
- `_CompileAndroidLibraryResources` now depends on
  `_ConvertLibraryResourcesCases`, and it should only run on the first
  batched instance of `_CompileAndroidLibraryResources`.
@jonathanpeppers jonathanpeppers mentioned this issue Nov 1, 2018
Merged
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Nov 2, 2018
@jonathanpeppers
[Xamarin.Android.Build.Tasks] aapt2, move ConvertResourcesCases to a …
7dd8cf4 
…new target

Fixes: dotnet#2356

When `$(AndroidUseAapt2)=True`, if you look for a log entry of a file
that is processed:

    Processing: C:\Users\myuser\Desktop\Git\xamarin-android\tests\Xamarin.Forms-Performance-Integration\Droid\obj\Debug\90\lp\22\jl\res\values\values.xml   10/30/2018 4:28:18 PM > 1/1/0001 12:00:00 AM

You will see this repeated several times (9 in case of a Xamarin.Forms
project).

It turns out, since the `_CompileAndroidLibraryResources` target is
batched, it is called once per item of
`@(_LibraryResourceHashDirectories)`:

    <Target Name="_CompileAndroidLibraryResources" DependsOnTargets="_CollectLibraryResourceDirectories"
            Condition="'$(_AndroidUseAapt2)' == 'True'"
            Inputs="%(_LibraryResourceHashDirectories.FileFound)"
            Outputs="$(_AndroidLibraryFlatArchivesDirectory)%(_LibraryResourceHashDirectories.Hash).stamp">
    ...
    <ConvertResourcesCases ... />
    <Aapt2Compile ... />
    ...
    </Target>

It calls `ConvertResourcesCases` for each call, which is not ideal.
`ConvertResourcesCases` operates on the whole tree of resources, so it
really should just run once: *before* `<Aapt2Compile/>`.

I think the fix here is:

- Move `ConvertResourcesCases` to a new
  `_ConvertLibraryResourcesCases` target
- The new target's `Inputs` should be
  `@(_LibraryResourceHashDirectories->'%(FileFound)')` and *not*
  batched.
- The new target's `Outputs` should be a new stamp file following our
  new convention.
- The new stamp file should also be used as
  `AndroidConversionFlagFile`.
- `_CompileAndroidLibraryResources` now depends on
  `_ConvertLibraryResourcesCases`, and it should only run on the first
  batched instance of `_CompileAndroidLibraryResources`.
dellis1972 pushed a commit that referenced this issue Nov 2, 2018
@jonathanpeppers @dellis1972
[Xamarin.Android.Build.Tasks] aapt2, move ConvertResourcesCases to a …
f6df332 
…new target (#2367)

Fixes: #2356

When `$(AndroidUseAapt2)=True`, if you look for a log entry of a file
that is processed:

    Processing: C:\Users\myuser\Desktop\Git\xamarin-android\tests\Xamarin.Forms-Performance-Integration\Droid\obj\Debug\90\lp\22\jl\res\values\values.xml   10/30/2018 4:28:18 PM > 1/1/0001 12:00:00 AM

You will see this repeated several times (9 in case of a Xamarin.Forms
project).

It turns out, since the `_CompileAndroidLibraryResources` target is
batched, it is called once per item of
`@(_LibraryResourceHashDirectories)`:

    <Target Name="_CompileAndroidLibraryResources" DependsOnTargets="_CollectLibraryResourceDirectories"
            Condition="'$(_AndroidUseAapt2)' == 'True'"
            Inputs="%(_LibraryResourceHashDirectories.FileFound)"
            Outputs="$(_AndroidLibraryFlatArchivesDirectory)%(_LibraryResourceHashDirectories.Hash).stamp">
    ...
    <ConvertResourcesCases ... />
    <Aapt2Compile ... />
    ...
    </Target>

It calls `ConvertResourcesCases` for each call, which is not ideal.
`ConvertResourcesCases` operates on the whole tree of resources, so it
really should just run once: *before* `<Aapt2Compile/>`.

I think the fix here is:

- Move `ConvertResourcesCases` to a new
  `_ConvertLibraryResourcesCases` target
- The new target's `Inputs` should be
  `@(_LibraryResourceHashDirectories->'%(FileFound)')` and *not*
  batched.
- The new target's `Outputs` should be a new stamp file following our
  new convention.
- The new stamp file should also be used as
  `AndroidConversionFlagFile`.
- `_CompileAndroidLibraryResources` now depends on
  `_ConvertLibraryResourcesCases`, and it should only run on the first
  batched instance of `_CompileAndroidLibraryResources`.
@ghost ghost locked as resolved and limited conversation to collaborators Jun 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@dellis1972 dellis1972

@jonathanpeppers jonathanpeppers

Labels
Area: Performance Issues with performance.
Projects
None yet
Milestone
d16-0
2 participants
@dellis1972 @jonathanpeppers