[Xamarin.Android.Build.Tasks] more ConvertResourcesCases improvements (#2328)

jonathanpeppers · dellis1972 · commit f7769efd3b14 · 2018-10-23T10:14:32.000+01:00
Context: https://www.getcodetrack.com/ I used a "freeware" .NET profiler to see what it would turn up. Using the tool, I profiled the following .NET process: .\bin\Debug\bin\xabuild.exe .\tests\Xamarin.Forms-Performance-Integration\Droid\Xamarin.Forms.Performance.Integration.Droid.csproj /v:quiet This seemed to have a lot of useful information, and I quickly noticed: Monodroid.AndroidResource.UpdateXmlResource -> 3683 calls -> 2.13s .. Monodroid.AndroidResource.ResourceNeedsToBeLowerCased -> 2384 calls - 319.53ms This is a known codepath, the `ConvertResourcesCases` MSBuild task, that we know takes a bit of time. Looking through the call stack, I saw two points we could make easy fixes for: - A `Regex` in `AndroidResource` was not using `RegexOptions.Compiled` - There was a string append `dirPrefix + "*"` in a LINQ expression that could be done up front. - There was a bit of LINQ usage such as: if (Directory.EnumerateDirectories (resourceBasePath, dirPrefix + "*").Any (dir => Directory.EnumerateFiles (dir, fileNamePattern).Any ())) So I added `RegexOptions.Compiled` and converted the LINQ expressions to `foreach` loops. I could see a noticeable change in these methods which moved further down the list of methods, when sorted by time duration. But since the time differences *in the profiler* likely aren't going to reflect the real world, I did a before/after comparision with the Xamarin.Forms.ControlGallery project. Project here: https://github.com/jonathanpeppers/Xamarin.Forms/tree/msbuild-timing/Xamarin.Forms.ControlGallery.Android Script here: https://github.com/jonathanpeppers/msbuild-timing/blob/master/xamarin.forms.ps1 Before: 7224 ms ConvertResourcesCases 6 calls After: 7034 ms ConvertResourcesCases 6 calls So about a 200ms improvement on Windows, for adjusting a bit of code. I suspect the improvement won't be as good on MacOS, since I don't believe `RegexOptions.Compiled` is implemented in Mono. (It is safely ignored) Other changes: - Code such as `(value ?? String.Empty).Trim ();` - Simplified to `value?.Trim ();` - Removed unused `filePath` variable Future changes: It may be worth auditing all regexes and adding `RegexOptions.Compiled` if they appear to be called a lot?
diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs b/src/Xamarin.Android.Build.Tasks/Utilities/AndroidResource.cs
@@ -37,7 +37,7 @@ public static bool UpdateXmlResource (string res, string filename, Dictionary<st
 
 		static readonly XNamespace android = "https://schemas.android.com/apk/res/android";
 		static readonly XNamespace res_auto = "https://schemas.android.com/apk/res-auto";
-		static readonly Regex r = new Regex (@"^@\+?(?<package>[^:]+:)?(anim|color|drawable|layout|menu)/(?<file>.*)$");
+		static readonly Regex r = new Regex (@"^@\+?(?<package>[^:]+:)?(anim|color|drawable|layout|menu)/(?<file>.*)$", RegexOptions.Compiled);
 		static readonly string[] fixResourcesAliasPaths = {
 			"/resources/item",
 			"/resources/integer-array/item",
@@ -104,38 +104,45 @@ static bool ResourceNeedsToBeLowerCased (string value, string resourceBasePath,
 			// Might be a bit of an overkill, but the data comes (indirectly) from the user since it's the
 			// path to the msbuild's intermediate output directory and that location can be changed by the
 			// user. It's better to be safe than sorry.
-			resourceBasePath = (resourceBasePath ?? String.Empty).Trim ();
-			if (String.IsNullOrEmpty (resourceBasePath))
+			resourceBasePath = resourceBasePath?.Trim ();
+			if (string.IsNullOrEmpty (resourceBasePath))
 				return true;
 
 			// Avoid resource names that are all whitespace
-			value = (value ?? String.Empty).Trim ();
-			if (String.IsNullOrEmpty (value))
+			value = value?.Trim ();
+			if (string.IsNullOrEmpty (value))
 				return false; // let's save some time
 			if (value.Length < 4 || value [0] != '@') // 4 is the minimum length since we need a string
 								  // that is at least of the following
 								  // form: @x/y. Checking it here saves some time
 								  // below.
 				return true;
 
-			string filePath = null;
 			int slash = value.IndexOf ('/');
 			int colon = value.IndexOf (':');
 			if (colon == -1)
 				colon = 0;
 
 			// Determine the the potential definition file's path based on the resource type.
-			string dirPrefix = value.Substring (colon + 1, slash - colon - 1).ToLowerInvariant ();
+			string dirPattern = value.Substring (colon + 1, slash - colon - 1).ToLowerInvariant () + "*";
 			string fileNamePattern = value.Substring (slash + 1).ToLowerInvariant () + ".*";
 			
-			if (Directory.EnumerateDirectories (resourceBasePath, dirPrefix + "*").Any (dir => Directory.EnumerateFiles (dir, fileNamePattern).Any ()))
-				return true;
+			foreach (var dir in Directory.EnumerateDirectories (resourceBasePath, dirPattern)) {
+				foreach (var file in Directory.EnumerateFiles (dir, fileNamePattern)) {
+					return true;
+				}
+			}
 
 			// check additional directories if we have them incase the resource is in a library project
-			if (additionalDirectories != null)
-				foreach (var additionalDirectory in additionalDirectories)
-					if (Directory.EnumerateDirectories (additionalDirectory, dirPrefix + "*").Any (dir => Directory.EnumerateFiles (dir, fileNamePattern).Any ()))
-						return true;
+			if (additionalDirectories != null) {
+				foreach (var additionalDirectory in additionalDirectories) {
+					foreach (var dir in Directory.EnumerateDirectories (additionalDirectory, dirPattern)) {
+						foreach (var file in Directory.EnumerateFiles (dir, fileNamePattern)) {
+							return true;
+						}
+					}
+				}
+			}
 
 			// No need to change the reference case.
 			return false;
