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] aapt2, move ConvertResourcesCases to a new target #2367

Merged

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Nov 1, 2018

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.

@dellis1972
Copy link
Contributor

@jonathanpeppers looks ok, do you have any data on how this effects the build times for Aapt2?

@jonathanpeppers
Copy link
Member Author

This won't help as much for "Hello World" since #2348.

So I timed the Xamarin.Forms Control Gallery:

Before:
500 ms  ConvertResourcesCases                     55 calls
After:
136 ms  ConvertResourcesCases                      9 calls

So this is going to help more with larger projects. #2348 already helped this issue alot.

Logs: logs.zip

…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 force-pushed the aapt2-convertresourcescases branch from 1f647ec to 7dd8cf4 Compare November 2, 2018 02:52
@dellis1972 dellis1972 merged commit f6df332 into dotnet:master Nov 2, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Feb 2, 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.

$(AndroidUseAapt2) runs ConvertResourcesCases a lot
3 participants