- 
                Notifications
    You must be signed in to change notification settings 
- Fork 561
[Xamarin.Android.Build.Tasks] mechanism to skip ConvertResourcesCases #2348
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] mechanism to skip ConvertResourcesCases #2348
Conversation
| Logs here: ConvertResourcesCases.zip | 
| I love this idea. It will bring major improvements for our customers. Perhaps we can have a backup which uses the assembly name/info which does a similar thing so that we can bring this improvement to people using older versions of the support libraries. | 
| If we want a build of the support libraries, they could add a file like this to their projects: //SkipAndroidResourceProcessingAttribute.cs
[assembly: SkipAndroidResourceProcessing]
namespace Xamarin.Android.Support
{
	[AttributeUsage (AttributeTargets.Assembly)]
	public sealed class SkipAndroidResourceProcessingAttribute : Attribute
	{
	}
}They could do this right now, even before an updated Xamarin.Android is out. (if an assembly attribute is the way we want to go) The  We can for sure get this in the 28.x version of the support libs, and we might need to backport it to the version that Xamarin.Forms is using. | 
        
          
                src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | Discussed this recently: We won't take this as-is. The problem with an assembly-level attribute which skips all processing of the resources contained within the assembly is that it's brittle. Imagine: 
 This is brittle. :-) Long term, we want to be able to use this optimization, but however we provide it, it needs to not be brittle in the face of the above scenario. | 
| We talked this through and we have a plan: Long TermWe figure out how to do this in a nicer way, where you won't as easily be able to mess it up. Someone could add the assembly attribute without knowing it will break their layouts. This might involve redoing how  Short TermWe are just going to "whitelist" assemblies to skip with an  <ItemGroup>
  <_AndroidAssemblySkipCases Include="Xamarin.Android.Support.v7.CardView" />
  <_AndroidAssemblySkipCases Include="Xamarin.Android.Support.v7.AppCompat" />
  <!--Etc-->
</ItemGroup>If we get in a tight spot, additional assemblies could be added/removed from this  Then our MSBuild tasks will treat  | 
b9e8b2e    to
    4de096d      
    Compare
  
    | Updated logs: ConvertResourcesCases2.zip | 
        
          
                src/Xamarin.Android.Build.Tasks/Tasks/ResolveLibraryProjectImports.cs
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | ******************************************* | ||
| --> | ||
| <!-- As we split up/refactor this file, put new imports here --> | ||
| <Import Project="$(MSBuildThisFileDirectory)Xamarin.Android.SkipCases.projitems" /> | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathanpeppers @jonpryor will this new file be picked up by the Mac and Visx installers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will have to add this file in the pkg in monodroid after this is merged.
df98d91    to
    7915e28      
    Compare
  
    | After using  Latest logs (set no. 3) here: ConvertResourcesCases3.zip | 
| Whoops, looking into the test failure | 
7915e28    to
    55736cc      
    Compare
  
    Currently, we run the `ConvertResourcesCases` MSBuild task for all
assemblies (on each assemby's `ResolveLibraryProjectImports` output).
`ConvertResourcesCases` is one of our slower targets.
Luckily, there are some "well-known" number of assemblies that we
could skip, so we have decided to "whitelist" certain assemblies such
as the Android support libraries, Google Play Services, Firebase, etc.
So a new `@(_AndroidAssemblySkipCases)` item group has been added such
as:
    <_AndroidAssemblySkipCases Include="Xamarin.Android.Support.v7.AppCompat" />
    <_AndroidAssemblySkipCases Include="Xamarin.Android.Support.v7.CardView" />
This `<ItemGroup/>` would be an indicator for `ConvertResourcesCases`
to just completely skip these assemblies.
Additionally, we can flat out skip `aar` files in the same manner.
To make this work:
- Added support to put `ITaskItem` metadata in the cache file produced
  by `ResolveLibraryProjectImports`
- Added item metadata for `SkipAndroidResourceProcessing` and
  `OriginalFile`.
- `ConvertResourcesCases` now skips these directories and logs
  `OriginalFile`.
- `CollectNonEmptyDirectories` needs to preserve item metadata for
  `$(AndroidUseAapt2)` to take advantage of the functionality.
The results appear to be well worth the effort!
Results with `$(AndroidUseAapt2)` enabled (note this is not the
default):
    Before:
    9912 ms  ConvertResourcesCases                      9 calls
    2219 ms  ResolveLibraryProjectImports               1 calls
    After:
      49 ms  ConvertResourcesCases                      9 calls
    2185 ms  ResolveLibraryProjectImports               1 calls
Results with `$(AndroidUseAapt2)` disabled:
    Before:
    1564 ms  ConvertResourcesCases                      1 calls
    1826 ms  ResolveLibraryProjectImports               1 calls
    After:
      25 ms  ConvertResourcesCases                      1 calls
    1685 ms  ResolveLibraryProjectImports               1 calls
This was the Xamarin.Forms-Integration project in this repo, an
initial clean build. It is basically a "Hello World" Xamarin.Forms
project. These updated numbers are from a `Release` build of
Xamarin.Android.
Overall this will save 1-2 seconds of `ConvertResourcesCases` for
default projects. This MSBuild task runs on an initial build or
incremental builds when Android resources have changed. I have gotten
slightly different numbers on the difference, each time I've compared.
There does not appear to be any noticeable slowdown in
`ResolveLibraryProjectImports` due to the changes.
    55736cc    to
    7a71c62      
    Compare
  
    
Currently, we run the
ConvertResourcesCasesMSBuild task for allassemblies (on each assemby's
ResolveLibraryProjectImportsoutput).ConvertResourcesCasesis one of our slower targets.Luckily, there are some "well-known" number of assemblies that we
could skip, so we have decided to "whitelist" certain assemblies such
as the Android support libraries, Google Play Services, Firebase, etc.
So a new
@(_AndroidAssemblySkipCases)item group has been added suchas:
This
<ItemGroup/>would be an indicator forConvertResourcesCasesto just completely skip these assemblies.
Additionally, we can flat out skip
aarfiles in the same manner.To make this work:
ITaskItemmetadata in the cache file producedby
ResolveLibraryProjectImportsSkipAndroidResourceProcessingandOriginalFile.ConvertResourcesCasesnow skips these directories and logsOriginalFile.CollectNonEmptyDirectoriesneeds to preserve item metadata for$(AndroidUseAapt2)to take advantage of the functionality.The results appear to be well worth the effort!
Results with
$(AndroidUseAapt2)enabled (note this is not thedefault):
Results with
$(AndroidUseAapt2)disabled:This was the Xamarin.Forms-Integration project in this repo, an
initial clean build. It is basically a "Hello World" Xamarin.Forms
project. These updated numbers are from a
Releasebuild ofXamarin.Android.
Overall this will save 1-2 seconds of
ConvertResourcesCasesfordefault projects. This MSBuild task runs on an initial build or
incremental builds when Android resources have changed. I have gotten
slightly different numbers on the difference, each time I've compared.
There does not appear to be any noticeable slowdown in
ResolveLibraryProjectImportsdue to the changes.