[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task by dellis1972 · Pull Request #2129 · dotnet/android

dellis1972 · 2018-08-31T16:41:47Z

Currently ConvertResourcesCases is called twice on ALL
resources referenced by the project. This is terribly inefficient.
However it is required. The first pass is done before a Compile,
this fixes up the casing for things like drawables etc ready to
be processed by aapt. The second is done after GenerateJavaStubs
which happens AFTER the Compile step. This is to replace any
custom view references with the correct {md5}.View style references.
This is because we replace the normal readable namespace with an md5 hash.

The problem was that ConvertResourcesCases is doing a TON of
work it doesn't really need to do the second time around. For example
checking if it needs to lower case names of items. The second pass
really only needs to worry about custom views. In addition to that
it was also re-scanning ALL the files again.

This commit introduces a new Task ConvertCustomView. The sole
purpose of this task is to fix up the layout files which contain
custom views. It does nothing else. To make this even quicker we
modify ConvertResourcesCases to emit a mapping file (class-map.txt). This file
contains items like

MonoDroid.Example.MyLayout;/fullpath/to/file.xml
android.support.v7.widget.ActionBarOverlayLayout;/fullpath/to/some/other/file.xml

This allows us to know were the files are which contain ANY layout.

With this information in conjuction with the acw_map.txt file will
allow us to do a targeted update. So we go through the acw_map.txt
values and fix up those files where we have entires in the class-map.txt.

This reduces the amount of time spent processing files quite a bit.
For a Blank Xamarin Forms app from a clean build.

2639 ms ConvertResourcesCases 1 calls
3 ms ConvertCustomView 1 calls

Normally the ConvertResourcesCases would be called twice and would
take a total of 5-6 seconds.

Fix up Paths in Error Message
Write a decent Commit Message
General Clean up.

dellis1972 · 2018-09-03T19:39:45Z

@jonathanpeppers we need a VSTS build on this. I'm not sure if the tempdst change will break stuff on windows. I will try to test locally tomorrow

jonathanpeppers · 2018-09-04T02:06:37Z

@dellis1972 I started one.

I had to update $(XABundleDownloadPrefix) to 'https://xamjenkinsartifact.azureedge.net/mono-jenkins/xamarin-android-1154/xamarin-android/bin/Debug', but it seemed like the first try failed building master. We'll see what happens.

jonathanpeppers · 2018-09-04T13:06:10Z

One test is failing on Windows:

Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5 / Debug
System.ArgumentNullException : Value cannot be null.
Parameter name: source
at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)
at Xamarin.Android.Tasks.ConvertCustomView.<>c__DisplayClass16_1.<Execute>b__0(TraceLevel t, String m) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 50
at Xamarin.Android.Tasks.ConvertCustomView.TryFixCustomView(XElement elem, Dictionary`2 acwMap, Action`2 logMessage) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 147
at Xamarin.Android.Tasks.ConvertCustomView.Execute() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 48
at Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\ConvertResourcesCasesTests.cs:line 91

dellis1972 · 2018-09-04T13:17:53Z

Ah yes, I need to update the unit tests to provide the new values (or check for nulls)

…

On Tue, 4 Sep 2018 at 14:06, Jonathan Peppers ***@***.***> wrote: One test is failing on Windows: Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5 / Debug System.ArgumentNullException : Value cannot be null. Parameter name: source at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate) at Xamarin.Android.Tasks.ConvertCustomView.<>c__DisplayClass16_1.<Execute>b__0(TraceLevel t, String m) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 50 at Xamarin.Android.Tasks.ConvertCustomView.TryFixCustomView(XElement elem, Dictionary`2 acwMap, Action`2 logMessage) in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 147 at Xamarin.Android.Tasks.ConvertCustomView.Execute() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tasks\ConvertCustomView.cs:line 48 at Xamarin.Android.Build.Tests.ConvertResourcesCasesTests.CheckClassIsNotReplacedWithMd5() in E:\A\_work\14\s\src\Xamarin.Android.Build.Tasks\Tests\Xamarin.Android.Build.Tests\ConvertResourcesCasesTests.cs:line 91 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2129 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxeeY0FsJkbGwVORitPERXhqLy3v2WEks5uXnrJgaJpZM4WVh5S> .

jonathanpeppers · 2018-09-04T13:18:33Z

+ map.Add (items [0], new HashSet<string> ());
+ if (!map[items [0]].Contains (items [1]))
+ map [items [0]].Add (items [1]);
+ }


If there are a lot of items in this dictionary, we should use some local variable so indexers aren't called as much.
Would help perf a little to be something like:
foreach(varsinFile.ReadLines(mapFile)){varsplit=s.Split(';');varkey=split[0];varvalue=split[1];if(!map.TryGetValue(key,outHashSet<string>items)){map[key]=items=newHashSet<string>();}if(!items.Contains(value)){items.Add(value);}}

jonpryor · 2018-09-04T15:08:38Z

+ var value = items [1];
+ if (!map.ContainsKey (key))
+ map.Add (key, new HashSet<string> ());
+ if (!map[key].Contains (value))


You don't need .Contains() here, as it's a HashSet<T>. Just always call map [key].Add(value).

jonpryor · 2018-09-04T15:10:35Z

+ var items = s.Split (';');
+ var key = items [0];
+ var value = items [1];
+ if (!map.ContainsKey (key))


This should instead do:
HashSet<string>set;if(!map.TryGetValue(key,outset)){map.Add(key,set=newHashSet<string>();}set.Add(value);

That is some nice code :)

jonpryor · 2018-09-04T15:18:38Z

+ if (!File.Exists (mapFile))
+ return map;
+ foreach (var s in File.ReadLines (mapFile)) {
+ var items = s.Split (';');


Don't use string.Split(char[]); use string.Split(char[], int):
varitems=s.Split(new[]{';'},2);
This way if the filesystem path contains a ; it'll behave sanely.

dellis1972 · 2018-09-04T15:23:38Z

need to change the name from class-map to customview-map and the associated Load/Save methods.

jonpryor · 2018-09-04T15:25:22Z

 break;
 }
- });
+ }, (e, filename) => {


Would it be possible to use named parameters here? I don't know what this callback is supposed to do.
},customViewMapCallback:(e,filename)=>{ ...

jonpryor · 2018-09-04T19:18:15Z

+ var temp = Path.GetTempFileName ();
+ try {
+ using (var m = new StreamWriter (temp)) {
+ foreach (var i in map) {


This should contain a map.OrderBy (k => k.Key) or something. Enumeration order of Dictionaries is undefined, so it's possible that two "identical" Dictionary<string, HashSet,string>> values will result in different orders in m, which means the CopyIfChanged() (below) becomes "useless."

jonpryor · 2018-09-04T19:18:55Z

+ try {
+ using (var m = new StreamWriter (temp)) {
+ foreach (var i in map) {
+ foreach (var v in i.Value)


Similarly, enumeration order of a HashSet<string> is undefined, so this should be i.Value.OrderBy (f => f) so that we can produce consistent files.

Fixeddotnet#2100 Currently `ConvertResourcesCases` is called twice on ALL resources referenced by the project. This is terribly inefficient. However it is required. The first pass is done before a `Compile`, this fixes up the casing for things like drawables etc ready to be processed by aapt. The second is done after `GenerateJavaStubs` which happens AFTER the `Compile` step. This is to replace any custom view references with the correct `{md5}.View` style references. This is because we replace the normal readable namespace with an md5 hash. The problem was that `ConvertResourcesCases` is doing a TON of work it doesn't really need to do the second time around. For example checking if it needs to lower case names of items. The second pass really only needs to worry about custom views. In addition to that it was also re-scanning ALL the files again. This commit introduces a new Task `ConvertCustomView`. The sole purpose of this task is to fix up the layout files which contain custom views. It does nothing else. To make this even quicker we modify `ConvertResourcesCases` to emit a mapping file (class-map.txt). This file contains items like MonoDroid.Example.MyLayout;/fullpath/to/file.xml android.support.v7.widget.ActionBarOverlayLayout;/fullpath/to/some/other/file.xml This allows us to know were the files are which contain ANY layout. With this information in conjuction with the `acw_map.txt` file will allow us to do a targeted update. So we go through the `acw_map.txt` values and fix up those files where we have entires in the `class-map.txt`. This reduces the amount of time spent processing files quite a bit. For a Blank Xamarin Forms app from a clean build. 2639 ms ConvertResourcesCases 1 calls 3 ms ConvertCustomView 1 calls Normally the `ConvertResourcesCases` would be called twice and would take a total of 5-6 seconds.

StephaneDelcroix · 2018-09-06T09:12:00Z

@@ -85,11 +98,32 @@ public override bool Execute ()
 var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
 var tmpdest = Path.GetTempFileName ();


I don't see this file being used before it gets deleted

Context: dotnet#2088 Context: dotnet#2129 Context: dotnet#2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.

Context: #2088 Context: #2129 Context: #2199 In 4deec52, I fixed the #deletebinobj problem we discovered. However... I introduced a regression to incremental builds, I noticed that the `_CompileJava` target is now running on a second build with no changes. Third build? oddly it gets skipped... It seems to be due to our use of flag files: 1. `_UpdateAndroidResgen` updates `R.cs.flag` and uses the file as an output 2. `_GenerateJavaDesignerForComponent` now uses `R.cs.flag` as an input 3. `_GenerateJavaStubs` *also* updates the timestamp on `R.cs.flag`. This was added in 970da9e, as a workaround for our two instances of `ConvertResourcesCases`. 4. `_GenerateJavaDesignerForComponent` will now run again on the next build. Since 1886e6f eliminated the second call to `ConvertResourcesCases`, we don't need to update `R.cs.flag` in no. 3 any longer. Removing the call to `<Touch />` `R.cs.flag` in `_GenerateJavaStubs` fixed the issue, and I added some assertions in relevant tests to check that the `_CompileJava` and `_GenerateJavaDesignerForComponent` targets aren't running on incremental builds.

dellis1972 added the do-not-merge PR should not be merged. label Aug 31, 2018

dellis1972 changed the title ~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP]Aug 31, 2018

dellis1972 force-pushed the Issue2100 branch 4 times, most recently from fcd102e to 10d1f6cCompare September 3, 2018 19:34

dellis1972 requested a review from jonathanpeppers September 3, 2018 19:39

dellis1972 removed the do-not-merge PR should not be merged. label Sep 3, 2018

dellis1972 changed the title ~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView Task [WIP]~~[Xamarin.Android.Build.Tasks] Add ConvertCustomView TaskSep 3, 2018

jonathanpeppers reviewed Sep 4, 2018
View reviewed changes

dellis1972 force-pushed the Issue2100 branch 3 times, most recently from ec65bdc to fca3b91Compare September 4, 2018 14:29

jonpryor reviewed Sep 4, 2018
View reviewed changes

dellis1972 force-pushed the Issue2100 branch from fca3b91 to 909b727Compare September 4, 2018 15:58

jonathanpeppers approved these changes Sep 4, 2018
View reviewed changes

jonpryor reviewed Sep 4, 2018
View reviewed changes

dellis1972 force-pushed the Issue2100 branch 2 times, most recently from db4cdf7 to ae22049Compare September 5, 2018 08:33

dellis1972 force-pushed the Issue2100 branch from ae22049 to f8a31a5Compare September 5, 2018 10:21

StephaneDelcroix reviewed Sep 5, 2018
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs

jonpryor suggested changes Sep 5, 2018
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs Outdated

jonpryor reviewed Sep 5, 2018
View reviewed changes

Comment threadsrc/Xamarin.Android.Build.Tasks/Tasks/ConvertCustomView.cs Outdated

dellis1972 added 2 commits September 5, 2018 16:18

Updated to unify the logging a bit
c9489ee

Fixed up Formatting
59fc89b

jonpryor merged commit 1886e6f into dotnet:masterSep 5, 2018

StephaneDelcroix reviewed Sep 6, 2018
View reviewed changes

jonathanpeppers mentioned this pull request Sep 21, 2018
[Xamarin.Android.Build.Tasks] fix _CompileJava running too often #2207
Merged

dellis1972 mentioned this pull request Oct 7, 2020
[Xamarin.Android.Build.Tasks] remove acw-map.txt usage in <ConvertResourcesCases/> #5184
Merged

github-actionsBot locked and limited conversation to collaborators Feb 2, 2024

		@@ -85,11 +98,32 @@ public override bool Execute ()
		var dstmodifiedDate = File.Exists (destfilename) ? File.GetLastWriteTimeUtc (destfilename) : DateTime.MinValue;
		var tmpdest = Path.GetTempFileName ();

Uh oh!

Conversation

dellis1972 commented Aug 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dellis1972 commented Sep 3, 2018

Uh oh!

jonathanpeppers commented Sep 4, 2018

Uh oh!

jonathanpeppers commented Sep 4, 2018

Uh oh!

dellis1972 commented Sep 4, 2018 via email

Uh oh!

jonathanpeppersSep 4, 2018

Choose a reason for hiding this comment

Uh oh!

jonpryorSep 4, 2018

Choose a reason for hiding this comment

Uh oh!

jonpryorSep 4, 2018

Choose a reason for hiding this comment

Uh oh!

dellis1972Sep 4, 2018

Choose a reason for hiding this comment

Uh oh!

jonpryorSep 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dellis1972 commented Sep 4, 2018

Uh oh!

jonpryorSep 4, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonpryorSep 4, 2018

Choose a reason for hiding this comment

Uh oh!

jonpryorSep 4, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

StephaneDelcroixSep 6, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dellis1972 commented Aug 31, 2018•
edited
Loading

jonpryorSep 4, 2018•
edited
Loading

jonpryorSep 4, 2018•
edited
Loading