From 9ea016747c21b6b7b1a267b825ff9bf7d292161c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Tue, 21 Apr 2020 15:22:59 +0200 Subject: [PATCH 1/3] Speed up linker by 10% ...using one weird trick. Cecil uses file I/O to read assemblies, but for our use cases this means we're spending a ton of time context switching between kernel mode and user mode. Memory mapped I/O is more efficient for these access patterns. The change is bigger than I would want it to be because: * Loading assemblies from disk is not centralized (and in fact there are more extensibility pointer that encourage everyone to do their own loading) * Cecil violates OOP principles by giving certain kinds of streams preferential treatment (`ModuleDefinition.FileName` is useless if the assembly wasn't opened with the blessed stream kind and Cecil doesn't have a way to provide it through other means). --- src/linker/Linker.Steps/OutputStep.cs | 4 +- src/linker/Linker/AssemblyResolver.cs | 9 +++- .../Linker/DirectoryAssemblyResolver.cs | 41 ++++++++++++++++++- src/linker/Linker/LinkContext.cs | 5 +-- 4 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/linker/Linker.Steps/OutputStep.cs b/src/linker/Linker.Steps/OutputStep.cs index c6cfd7bf0a0c..f71fbc32a88c 100644 --- a/src/linker/Linker.Steps/OutputStep.cs +++ b/src/linker/Linker.Steps/OutputStep.cs @@ -257,9 +257,9 @@ static string GetConfigFile (string assembly) return assembly + ".config"; } - static FileInfo GetOriginalAssemblyFileInfo (AssemblyDefinition assembly) + FileInfo GetOriginalAssemblyFileInfo (AssemblyDefinition assembly) { - return new FileInfo (assembly.MainModule.FileName); + return new FileInfo (Context.Resolver.AssemblyToPath [assembly]); } protected virtual void CopyAssembly (AssemblyDefinition assembly, string directory) diff --git a/src/linker/Linker/AssemblyResolver.cs b/src/linker/Linker/AssemblyResolver.cs index f41fe61d66ea..3aa5e97ec907 100644 --- a/src/linker/Linker/AssemblyResolver.cs +++ b/src/linker/Linker/AssemblyResolver.cs @@ -99,6 +99,11 @@ AssemblyDefinition ResolveFromReferences (AssemblyNameReference name, Collection return null; } + public AssemblyDefinition ResolveFromPath(string path, ReaderParameters parameters) + { + return CacheAssembly (GetAssembly (path, parameters)); + } + public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderParameters parameters) { // Validate arguments, similarly to how the base class does it. @@ -133,7 +138,7 @@ public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderPa public virtual AssemblyDefinition CacheAssembly (AssemblyDefinition assembly) { _assemblies [assembly.Name.Name] = assembly; - base.AddSearchDirectory (Path.GetDirectoryName (assembly.MainModule.FileName)); + base.AddSearchDirectory (Path.GetDirectoryName (AssemblyToPath[assembly])); return assembly; } @@ -151,6 +156,8 @@ protected override void Dispose (bool disposing) _assemblies.Clear (); if (_unresolvedAssemblies != null) _unresolvedAssemblies.Clear (); + + base.Dispose (disposing); } } } diff --git a/src/linker/Linker/DirectoryAssemblyResolver.cs b/src/linker/Linker/DirectoryAssemblyResolver.cs index 5b9a95be92cd..002796a06044 100644 --- a/src/linker/Linker/DirectoryAssemblyResolver.cs +++ b/src/linker/Linker/DirectoryAssemblyResolver.cs @@ -7,6 +7,7 @@ using System.IO; using Mono.Collections.Generic; using Mono.Cecil; +using System.IO.MemoryMappedFiles; #if FEATURE_ILLINK namespace Mono.Linker { @@ -15,6 +16,10 @@ public abstract class DirectoryAssemblyResolver : IAssemblyResolver { readonly Collection directories; + public readonly Dictionary AssemblyToPath = new Dictionary (); + + readonly List viewStreams = new List (); + public void AddSearchDirectory (string directory) { directories.Add (directory); @@ -40,7 +45,34 @@ protected AssemblyDefinition GetAssembly (string file, ReaderParameters paramete if (parameters.AssemblyResolver == null) parameters.AssemblyResolver = this; - return ModuleDefinition.ReadModule (file, parameters).Assembly; + FileStream fileStream = null; + MemoryMappedFile mappedFile = null; + MemoryMappedViewStream viewStream = null; + try { + // Create stream because CreateFromFile(string, ...) uses FileShare.None which is too strict + fileStream = new FileStream (file, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, false); + mappedFile = MemoryMappedFile.CreateFromFile ( + fileStream, null, fileStream.Length, MemoryMappedFileAccess.Read, HandleInheritability.None, true); + viewStream = mappedFile.CreateViewStream (0, 0, MemoryMappedFileAccess.Read); + + AssemblyDefinition result = ModuleDefinition.ReadModule (viewStream, parameters).Assembly; + + AssemblyToPath.Add (result, file); + + viewStreams.Add (viewStream); + + // We transferred the ownership of the viewStream to the collection. + viewStream = null; + + return result; + } finally { + if (fileStream != null) + fileStream.Dispose (); + if (mappedFile != null) + mappedFile.Dispose (); + if (viewStream != null) + viewStream.Dispose (); + } } public virtual AssemblyDefinition Resolve (AssemblyNameReference name) @@ -89,6 +121,13 @@ public void Dispose () protected virtual void Dispose (bool disposing) { + if (disposing) { + foreach (var viewStream in viewStreams) { + viewStream.Dispose (); + } + + viewStreams.Clear (); + } } } } diff --git a/src/linker/Linker/LinkContext.cs b/src/linker/Linker/LinkContext.cs index b4ae0fda8008..79ae8ccfec1e 100644 --- a/src/linker/Linker/LinkContext.cs +++ b/src/linker/Linker/LinkContext.cs @@ -264,8 +264,7 @@ public AssemblyDefinition Resolve (string name) { if (File.Exists (name)) { try { - AssemblyDefinition assembly = AssemblyDefinition.ReadAssembly (name, _readerParameters); - return _resolver.CacheAssembly (assembly); + return _resolver.ResolveFromPath (name, _readerParameters); } catch (Exception e) { throw new AssemblyResolutionException (new AssemblyNameReference (name, new Version ()), e); } @@ -314,7 +313,7 @@ public virtual void SafeReadSymbols (AssemblyDefinition assembly) try { var symbolReader = _symbolReaderProvider.GetSymbolReader ( assembly.MainModule, - assembly.MainModule.FileName); + _resolver.AssemblyToPath[assembly]); if (symbolReader == null) return; From 5cbd37edf00f26a308b74cd64902e7a30b441aaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 22 Apr 2020 12:08:12 +0200 Subject: [PATCH 2/3] Review feedback --- src/linker/Linker.Steps/OutputStep.cs | 2 +- src/linker/Linker/AssemblyResolver.cs | 16 +++++++++++++++- src/linker/Linker/DirectoryAssemblyResolver.cs | 14 ++++---------- src/linker/Linker/LinkContext.cs | 2 +- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/linker/Linker.Steps/OutputStep.cs b/src/linker/Linker.Steps/OutputStep.cs index f71fbc32a88c..22b31be66678 100644 --- a/src/linker/Linker.Steps/OutputStep.cs +++ b/src/linker/Linker.Steps/OutputStep.cs @@ -259,7 +259,7 @@ static string GetConfigFile (string assembly) FileInfo GetOriginalAssemblyFileInfo (AssemblyDefinition assembly) { - return new FileInfo (Context.Resolver.AssemblyToPath [assembly]); + return new FileInfo (Context.Resolver.GetAssemblyFileName (assembly)); } protected virtual void CopyAssembly (AssemblyDefinition assembly, string directory) diff --git a/src/linker/Linker/AssemblyResolver.cs b/src/linker/Linker/AssemblyResolver.cs index 3aa5e97ec907..9a423ab7782a 100644 --- a/src/linker/Linker/AssemblyResolver.cs +++ b/src/linker/Linker/AssemblyResolver.cs @@ -83,6 +83,20 @@ AssemblyDefinition GetAssembly (string file, ReaderParameters parameters) } #endif + public string GetAssemblyFileName(AssemblyDefinition assembly) + { +#if FEATURE_ILLINK + if (assemblyToPath.TryGetValue(assembly, out string path)) { + return path; + } + else +#endif + { + // Must be an assembly that we didn't open through the resolver + return assembly.MainModule.FileName; + } + } + AssemblyDefinition ResolveFromReferences (AssemblyNameReference name, Collection references, ReaderParameters parameters) { var fileName = name.Name + ".dll"; @@ -138,7 +152,7 @@ public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderPa public virtual AssemblyDefinition CacheAssembly (AssemblyDefinition assembly) { _assemblies [assembly.Name.Name] = assembly; - base.AddSearchDirectory (Path.GetDirectoryName (AssemblyToPath[assembly])); + base.AddSearchDirectory (Path.GetDirectoryName (GetAssemblyFileName(assembly))); return assembly; } diff --git a/src/linker/Linker/DirectoryAssemblyResolver.cs b/src/linker/Linker/DirectoryAssemblyResolver.cs index 002796a06044..edf1add2c0cf 100644 --- a/src/linker/Linker/DirectoryAssemblyResolver.cs +++ b/src/linker/Linker/DirectoryAssemblyResolver.cs @@ -16,7 +16,7 @@ public abstract class DirectoryAssemblyResolver : IAssemblyResolver { readonly Collection directories; - public readonly Dictionary AssemblyToPath = new Dictionary (); + protected readonly Dictionary assemblyToPath = new Dictionary (); readonly List viewStreams = new List (); @@ -45,19 +45,17 @@ protected AssemblyDefinition GetAssembly (string file, ReaderParameters paramete if (parameters.AssemblyResolver == null) parameters.AssemblyResolver = this; - FileStream fileStream = null; - MemoryMappedFile mappedFile = null; MemoryMappedViewStream viewStream = null; try { // Create stream because CreateFromFile(string, ...) uses FileShare.None which is too strict - fileStream = new FileStream (file, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, false); - mappedFile = MemoryMappedFile.CreateFromFile ( + using var fileStream = new FileStream (file, FileMode.Open, FileAccess.Read, FileShare.Read, 4096, false); + using var mappedFile = MemoryMappedFile.CreateFromFile ( fileStream, null, fileStream.Length, MemoryMappedFileAccess.Read, HandleInheritability.None, true); viewStream = mappedFile.CreateViewStream (0, 0, MemoryMappedFileAccess.Read); AssemblyDefinition result = ModuleDefinition.ReadModule (viewStream, parameters).Assembly; - AssemblyToPath.Add (result, file); + assemblyToPath.Add (result, file); viewStreams.Add (viewStream); @@ -66,10 +64,6 @@ protected AssemblyDefinition GetAssembly (string file, ReaderParameters paramete return result; } finally { - if (fileStream != null) - fileStream.Dispose (); - if (mappedFile != null) - mappedFile.Dispose (); if (viewStream != null) viewStream.Dispose (); } diff --git a/src/linker/Linker/LinkContext.cs b/src/linker/Linker/LinkContext.cs index 79ae8ccfec1e..0502ca3293df 100644 --- a/src/linker/Linker/LinkContext.cs +++ b/src/linker/Linker/LinkContext.cs @@ -313,7 +313,7 @@ public virtual void SafeReadSymbols (AssemblyDefinition assembly) try { var symbolReader = _symbolReaderProvider.GetSymbolReader ( assembly.MainModule, - _resolver.AssemblyToPath[assembly]); + _resolver.GetAssemblyFileName(assembly)); if (symbolReader == null) return; From 597f15741551ffb55cbca3e4a1852fc0ef1d36a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 23 Apr 2020 09:33:12 +0200 Subject: [PATCH 3/3] Review feedback --- src/linker/Linker/DirectoryAssemblyResolver.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker/DirectoryAssemblyResolver.cs b/src/linker/Linker/DirectoryAssemblyResolver.cs index edf1add2c0cf..c73f5fa608e2 100644 --- a/src/linker/Linker/DirectoryAssemblyResolver.cs +++ b/src/linker/Linker/DirectoryAssemblyResolver.cs @@ -5,9 +5,9 @@ using System; using System.Collections.Generic; using System.IO; +using System.IO.MemoryMappedFiles; using Mono.Collections.Generic; using Mono.Cecil; -using System.IO.MemoryMappedFiles; #if FEATURE_ILLINK namespace Mono.Linker {