-
Notifications
You must be signed in to change notification settings - Fork 63
[Java.Interop.Tools.Cecil] use File.Exists instead of DirectoryGetFile #596
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
Merged
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:remove-directorygetfile
Mar 9, 2020
Merged
[Java.Interop.Tools.Cecil] use File.Exists instead of DirectoryGetFile #596
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:remove-directorygetfile
Mar 9, 2020
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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:Resolve` is called thousands of times during a build. It makes sense 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.
jpobst
approved these changes
Mar 9, 2020
Member
Author
|
I was puzzled at how this simple change helps so much, but it looks like after I add some logging In a first build, both |
jonpryor
pushed a commit
that referenced
this pull request
Mar 16, 2020
#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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
As seen in b81cfbb,
DirectoryAssemblyResolver:Resolveis calledthousands of times during a build. It makes sense this would show up
in a profiler?
Looking at the code:
It seems this will always allocate a
string[], and it's callingDirectory.GetFilesusing the filename as a wildcard. I thought aFile.Existscheck would be better?Comparing the two methods with Benchmark.NET:
https://github.com/jonathanpeppers/Benchmarks/blob/d36d72eaf7fc82f20137c47ff768ffc38392f938/Benchmarks/FilesBenchmarks.cs
It seems that
File.Existswill be ~60% better.After making that change, I saw an improvement when building the
Xamarin.Forms integration project on Windows:
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.