Skip to content

Commit

Permalink
[Java.Interop.Tools.Cecil] use File.Exists instead of DirectoryGetFile (
Browse files Browse the repository at this point in the history
dotnet#596)

Context: https://www.jetbrains.com/profiler/

I have been playing with dotTrace, and it displayed the following as
the no. 4 top "hot path" during a build of the Xamarin.Forms
integration project:

	0.08%   DirectoryGetFile  •  376/0 ms  •  Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.DirectoryGetFile(String, String)
	  0.08%   SearchDirectory  •  Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.SearchDirectory(String, String)
	    0.08%   Resolve  •  Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(AssemblyNameReference, ReaderParameters)
	      0.05%   RunTask  •  262 of 376 ms  •  Xamarin.Android.Tasks.LinkAssembliesNoShrink.RunTask
	      0.02%   Resolve  •  114 of 376 ms  •  Java.Interop.Tools.Cecil.DirectoryAssemblyResolver.Resolve(AssemblyNameReference)

As seen in b81cfbb, `DirectoryAssemblyResolver:Resolv.()` is called
thousands of times during a build. Does it makes sense that this would
show up in a profiler?

Looking at the code:

	static string DirectoryGetFile (string directory, string file)
	{
	    if (!Directory.Exists (directory))
	        return "";

	    var files = Directory.GetFiles (directory, file);
	    if (files != null && files.Length > 0)
	        return files [0];

	    return "";
	}

It seems this will always allocate a `string[]`, and it's calling
`Directory.GetFiles()` using the filename as a wildcard.  I thought a
`File.Exists` check would be better?

Comparing the two methods with Benchmark.NET:

https://github.com/jonathanpeppers/Benchmarks/blob/d36d72eaf7fc82f20137c47ff768ffc38392f938/Benchmarks/FilesBenchmarks.cs

	BenchmarkDotNet=v0.12.0, OS=Windows 10.0.18362
	Intel Core i9-9900K CPU 3.60GHz (Coffee Lake), 1 CPU, 16 logical and 8 physical cores
	[Host]     : .NET Framework 4.8 (4.8.4121.0), X86 LegacyJIT
	|  Method          |     Mean |    Error |   StdDev |  Gen 0 | Gen 1 | Gen 2 | Allocated |
	|--------          |---------:|---------:|---------:|-------:|------:|------:|----------:|
	| File.Exists      | 20.88 us | 0.393 us | 0.349 us | 0.0305 |     - |     - |     297 B |
	| DirectoryGetFile | 56.28 us | 0.308 us | 0.288 us | 0.2441 |     - |     - |    1282 B |

It seems that `File.Exists()` will be ~60% better.

After making that change, I saw an improvement when building the
Xamarin.Forms integration project on Windows:

  * Before:

        711 ms  LinkAssembliesNoShrink                     1 calls

  * After:

        426 ms  LinkAssembliesNoShrink                     1 calls

It seems like this will save ~290ms on the initial build.  This target
builds partially on incremental builds, so we would see a smaller
improvement there.
  • Loading branch information
jonathanpeppers authored Mar 9, 2020
1 parent 27cfd45 commit 0a3354b
Showing 1 changed file with 4 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -254,27 +254,15 @@ string SearchDirectory (string name, string directory)
if (Path.IsPathRooted (name) && File.Exists (name))
return name;

var file = DirectoryGetFile (directory, name + ".dll");
if (file.Length > 0)
var file = Path.Combine (directory, name + ".dll");
if (File.Exists (file))
return file;

file = DirectoryGetFile (directory, name + ".exe");
if (file.Length > 0)
file = Path.Combine (directory, name + ".exe");
if (File.Exists (file))
return file;

return null;
}

static string DirectoryGetFile (string directory, string file)
{
if (!Directory.Exists (directory))
return "";

var files = Directory.GetFiles (directory, file);
if (files != null && files.Length > 0)
return files [0];

return "";
}
}
}

0 comments on commit 0a3354b

Please sign in to comment.