Skip to content

Commit

Permalink
Fix analyzer conflicts on Linux (dotnet#69406)
Browse files Browse the repository at this point in the history
This changes analyzer loading on Linux to be per user where before clashes could occur in the temp directory.

This fix initially started out by making our temp directory logic more robust. Essentially add per user sub directories, do extended permission checks, etc ... That exacerbated a number of existing corner cases in the code and forced us to put a lot more error handling into the workspaces layer.

After discussion with the IDE team I decided to take the approach of loading from a `Stream` on non-Windows platforms. The performance difference is neglible (1ms to 2ms per assembly) and it removes the overhead of copying them around on disk as well as the complexities that come with managing the shadow copy directory.

This will be observable to any analyzer that used `Assembly.Location` as it will now be `""`. That is only useful though for inspecting disks which is already illegal for analyzers hence it's considered an acceptable break.

To test the performance of this change I wrote a simple benchmark that loads assemblies directly from path, copy then load (simulate shadow copy) and loading them from stream. The results on Windows is pretty striking

|Method | Mean | Error | StdDev | Median|
-- | -- | -- | -- | --| 
|LoadViaPath | 1.552 ms | 0.0387 ms | 0.1142 ms | 1.529 ms|
|LoadViaPathShadow | 33.147 ms | 0.9000 ms | 2.6537 ms | 33.883 ms|
|LoadViaStream | 495.945 ms | 9.5040 ms | 9.7599 ms | 493.297 ms|

Notice the penalty of `LoadFromStream` is severe on Windows. That is due to the AV having to run on every load since it can't cache like it can when loading from path. On Linux though the results are much nicer

|Method | Mean | Error | StdDev | Median|
|-- | -- | -- | -- | --|
|LoadViaPath | 1.301 ms | 0.0258 ms | 0.0326 ms | 1.299 ms|
|LoadViaPathShadow | 4.126 ms | 0.3567 ms | 1.0060 ms | 3.742 ms|
|LoadViaStream | 2.514 ms | 0.0501 ms | 0.0536 ms | 2.497 m|

This means the change should be a small win on Linux but probably not in the noticable range. 

closes dotnet#65415
  • Loading branch information
jaredpar authored Aug 9, 2023
1 parent b1afd52 commit f7cb1f2
Show file tree
Hide file tree
Showing 11 changed files with 235 additions and 125 deletions.
10 changes: 9 additions & 1 deletion docs/Breaking API Changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,4 +87,12 @@ All `SymbolDisplayFormat`s (predefined and user-created) now include parameter n

`IncrementalGeneratorRunStep.Outputs` previously contained `IncrementalStepRunReason.Modified` as `Reason`
when the input to the step was modified in a way that produced a new output.
Now the reason will be reported more accurately as `IncrementalStepRunReason.New`.
Now the reason will be reported more accurately as `IncrementalStepRunReason.New`.

# Version 4.8.0

### Changed `Assembly.Location` behavior in non-Windows

The value of `Assembly.Location` previously held the location on disk where an analyzer or source generator was loaded from. This could be either the original location or the shadow copy location. In 4.8 this will be `""` in certain cases when running on non Windows platforms. This is due the compiler server loading assemblies using `AssemblyLoadContext.LoadFromStream` instead of loading from disk.

This could already happen in other load scenarios but this change moves it into mainline build scenarios.
183 changes: 103 additions & 80 deletions src/Compilers/Core/CodeAnalysisTest/AnalyzerAssemblyLoaderTests.cs

Large diffs are not rendered by default.

24 changes: 16 additions & 8 deletions src/Compilers/Core/CodeAnalysisTest/InvokeUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ namespace Microsoft.CodeAnalysis.UnitTests

public sealed class InvokeUtil
{
public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadContext compilerContext, AssemblyLoadTestFixture fixture, bool shadowLoad, string typeName, string methodName)
public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadContext compilerContext, AssemblyLoadTestFixture fixture, AnalyzerTestKind kind, string typeName, string methodName)
{
// Ensure that the test did not load any of the test fixture assemblies into
// the default load context. That should never happen. Assemblies should either
Expand All @@ -46,9 +46,14 @@ public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadContext compile
var compilerContextCount = compilerContext.Assemblies.Count();

using var tempRoot = new TempRoot();
AnalyzerAssemblyLoader loader = shadowLoad
? new ShadowCopyAnalyzerAssemblyLoader(compilerContext, tempRoot.CreateDirectory().Path)
: new DefaultAnalyzerAssemblyLoader(compilerContext);
AnalyzerAssemblyLoader loader = kind switch
{
AnalyzerTestKind.LoadDirect => new DefaultAnalyzerAssemblyLoader(compilerContext, AnalyzerLoadOption.LoadFromDisk),
AnalyzerTestKind.LoadStream => new DefaultAnalyzerAssemblyLoader(compilerContext, AnalyzerLoadOption.LoadFromStream),
AnalyzerTestKind.ShadowLoad => new ShadowCopyAnalyzerAssemblyLoader(compilerContext, tempRoot.CreateDirectory().Path),
_ => throw ExceptionUtilities.Unreachable()
};

try
{
AnalyzerAssemblyLoaderTests.InvokeTestCode(loader, fixture, typeName, methodName);
Expand Down Expand Up @@ -88,12 +93,15 @@ public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadContext compile

public sealed class InvokeUtil : MarshalByRefObject
{
public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadTestFixture fixture, bool shadowLoad, string typeName, string methodName)
public void Exec(ITestOutputHelper testOutputHelper, AssemblyLoadTestFixture fixture, AnalyzerTestKind kind, string typeName, string methodName)
{
using var tempRoot = new TempRoot();
AnalyzerAssemblyLoader loader = shadowLoad
? new ShadowCopyAnalyzerAssemblyLoader(tempRoot.CreateDirectory().Path)
: new DefaultAnalyzerAssemblyLoader();
AnalyzerAssemblyLoader loader = kind switch
{
AnalyzerTestKind.LoadDirect => new DefaultAnalyzerAssemblyLoader(),
AnalyzerTestKind.ShadowLoad => new ShadowCopyAnalyzerAssemblyLoader(tempRoot.CreateDirectory().Path),
_ => throw ExceptionUtilities.Unreachable()
};

try
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

Expand All @@ -11,19 +11,46 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Runtime.Loader;

namespace Microsoft.CodeAnalysis
{
internal enum AnalyzerLoadOption
{
/// <summary>
/// Once the assembly path is chosen, load it directly from disk at that location
/// </summary>
LoadFromDisk,

/// <summary>
/// Once the assembly path is chosen, read the contents of disk and load from memory
/// </summary>
/// <remarks>
/// While Windows supports this option it comes with a significant performance penalty due
/// to anti virus scans. It can have a load time of 300-500ms while loading from disk
/// is generally 1-2ms. Use this with caution on Windows.
/// </remarks>
LoadFromStream
}

internal partial class AnalyzerAssemblyLoader
{
private readonly AssemblyLoadContext _compilerLoadContext;
private readonly Dictionary<string, DirectoryLoadContext> _loadContextByDirectory = new Dictionary<string, DirectoryLoadContext>(StringComparer.Ordinal);
private readonly AnalyzerLoadOption _loadOption;

internal AssemblyLoadContext CompilerLoadContext => _compilerLoadContext;
internal AnalyzerLoadOption AnalyzerLoadOption => _loadOption;

internal AnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext = null)
internal AnalyzerAssemblyLoader()
: this(null, AnalyzerLoadOption.LoadFromDisk)
{
}

internal AnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext, AnalyzerLoadOption loadOption)
{
_loadOption = loadOption;
_compilerLoadContext = compilerLoadContext ?? AssemblyLoadContext.GetLoadContext(typeof(AnalyzerAssemblyLoader).GetTypeInfo().Assembly)!;
}

Expand Down Expand Up @@ -105,7 +132,7 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader, Ass
if (_loader.IsAnalyzerDependencyPath(assemblyPath))
{
(_, var loadPath) = _loader.GetAssemblyInfoForPath(assemblyPath);
return LoadFromAssemblyPath(loadPath);
return loadCore(loadPath);
}

// Next prefer registered dependencies from other directories. Ideally this would not
Expand All @@ -114,11 +141,24 @@ public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader, Ass
// https://github.com/dotnet/roslyn/issues/56442
if (_loader.GetBestPath(assemblyName) is string bestRealPath)
{
return LoadFromAssemblyPath(bestRealPath);
return loadCore(bestRealPath);
}

// No analyzer registered this dependency. Time to fail
return null;

Assembly loadCore(string assemblyPath)
{
if (_loader.AnalyzerLoadOption == AnalyzerLoadOption.LoadFromDisk)
{
return LoadFromAssemblyPath(assemblyPath);
}
else
{
using var stream = File.Open(assemblyPath, FileMode.Open, FileAccess.Read, FileShare.Read);
return LoadFromStream(stream);
}
}
}

protected override IntPtr LoadUnmanagedDll(string unmanagedDllName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Runtime.InteropServices;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis
Expand All @@ -17,14 +18,18 @@ internal sealed class DefaultAnalyzerAssemblyLoader : AnalyzerAssemblyLoader
{
#if NETCOREAPP

// Called from a netstandard2.0 project, so need to ensure a parameterless constructor is available.
internal DefaultAnalyzerAssemblyLoader()
: this(null)
{
}

internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null)
: base(compilerLoadContext)
internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null, AnalyzerLoadOption loadOption = AnalyzerLoadOption.LoadFromDisk)
: base(compilerLoadContext, loadOption)
{
}

#else

internal DefaultAnalyzerAssemblyLoader()
{
}

Expand All @@ -36,5 +41,42 @@ internal DefaultAnalyzerAssemblyLoader(System.Runtime.Loader.AssemblyLoadContext
/// <param name="fullPath"></param>
/// <returns></returns>
protected override string PreparePathToLoad(string fullPath) => fullPath;

/// <summary>
/// Return an <see cref="IAnalyzerAssemblyLoader"/> which does not lock assemblies on disk that is
/// most appropriate for the current platform.
/// </summary>
/// <param name="windowsShadowPath">A shadow copy path will be created on Windows and this value
/// will be the base directory where shadow copy assemblies are stored. </param>
internal static IAnalyzerAssemblyLoader CreateNonLockingLoader(string windowsShadowPath)
{
#if NETCOREAPP
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
// The cost of doing stream based loading on Windows is too expensive and we must continue to
// use the shadow copy loader.
return createShadowLoaderWindows();
}
else
{
return new DefaultAnalyzerAssemblyLoader(loadOption: AnalyzerLoadOption.LoadFromStream);
}
#else
return createShadowLoaderWindows();
#endif

ShadowCopyAnalyzerAssemblyLoader createShadowLoaderWindows()
{
// The shadow copy analyzer should only be created on Windows. To create on Linux we cannot use
// GetTempPath as it's not per-user. Generally there is no need as LoadFromStream achieves the same
// effect
if (!Path.IsPathRooted(windowsShadowPath))
{
throw new ArgumentException("Must be a full path.", nameof(windowsShadowPath));
}

return new ShadowCopyAnalyzerAssemblyLoader(windowsShadowPath);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,30 +44,23 @@ internal sealed class ShadowCopyAnalyzerAssemblyLoader : AnalyzerAssemblyLoader
internal int CopyCount => _assemblyDirectoryId;

#if NETCOREAPP
public ShadowCopyAnalyzerAssemblyLoader(string? baseDirectory = null)
public ShadowCopyAnalyzerAssemblyLoader(string baseDirectory)
: this(null, baseDirectory)
{
}

public ShadowCopyAnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext, string? baseDirectory = null)
: base(compilerLoadContext)
public ShadowCopyAnalyzerAssemblyLoader(AssemblyLoadContext? compilerLoadContext, string baseDirectory)
: base(compilerLoadContext, AnalyzerLoadOption.LoadFromDisk)
#else
public ShadowCopyAnalyzerAssemblyLoader(string? baseDirectory = null)
public ShadowCopyAnalyzerAssemblyLoader(string baseDirectory)
#endif
{
if (baseDirectory != null)
if (baseDirectory is null)
{
_baseDirectory = baseDirectory;
}
else
{
// https://github.com/dotnet/roslyn/issues/65415
// Fixing that issue will involve removing this GetTempPath call
#pragma warning disable RS0030
_baseDirectory = Path.Combine(Path.GetTempPath(), "CodeAnalysis", "AnalyzerShadowCopies");
#pragma warning restore RS0030
throw new ArgumentNullException(nameof(baseDirectory));
}

_baseDirectory = baseDirectory;
_shadowCopyDirectoryAndMutex = new Lazy<(string directory, Mutex)>(
() => CreateUniqueDirectoryForProcess(), LazyThreadSafetyMode.ExecutionAndPublication);

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/Server/VBCSCompiler/CompilerRequestHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public RunRequest(Guid requestId, string language, string? workingDirectory, str

internal sealed class CompilerServerHost : ICompilerServerHost
{
public IAnalyzerAssemblyLoader AnalyzerAssemblyLoader { get; } = new ShadowCopyAnalyzerAssemblyLoader(baseDirectory: Path.Combine(Path.GetTempPath(), "VBCSCompiler", "AnalyzerAssemblyLoader"));
public IAnalyzerAssemblyLoader AnalyzerAssemblyLoader { get; }

public static Func<string, MetadataReferenceProperties, PortableExecutableReference> SharedAssemblyReferenceProvider { get; } = (path, properties) => new CachingMetadataReference(path, properties);

Expand Down Expand Up @@ -72,6 +72,7 @@ internal CompilerServerHost(string clientDirectory, string? sdkDirectory, ICompi
ClientDirectory = clientDirectory;
SdkDirectory = sdkDirectory;
Logger = logger;
AnalyzerAssemblyLoader = DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader(Path.Combine(Path.GetTempPath(), "VBCSCompiler", "AnalyzerAssemblyLoader"));
}

public bool TryCreateCompiler(in RunRequest request, BuildPaths buildPaths, [NotNullWhen(true)] out CommonCompiler? compiler)
Expand Down
10 changes: 0 additions & 10 deletions src/Compilers/Shared/BuildServerConnection.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ internal sealed class BuildServerConnection
/// <summary>
/// Create a build request for processing on the server.
/// </summary>
/// <remarks>
/// Even though compilation itself does not specifically require a temporary directory to be successful
/// this API deliberately requires <paramref name="tempDirectory"/> to have a value. The reason for this is
/// that the server itself requires a temporary directory to startup and it uses the same logic as the client
/// to calculate it.
///
/// That means if the client can't calculate the temporary directory the server won't be able to either and
/// hence won't be able to start. So creating <see cref="BuildRequest"/> instances isn't useful cause it's
/// wasting time trying to start a server that won't be able to start.
/// </remarks>
internal static BuildRequest CreateBuildRequest(
Guid requestId,
RequestLanguage language,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,16 @@
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.Diagnostics;
using System.IO;

namespace Microsoft.CodeAnalysis.ExternalAccess.OmniSharp.Analyzers
{
internal static class OmnisharpAnalyzerAssemblyLoaderFactory
{
public static IAnalyzerAssemblyLoader CreateShadowCopyAnalyzerAssemblyLoader(string? baseDirectory = null)
{
return new ShadowCopyAnalyzerAssemblyLoader(baseDirectory);
baseDirectory ??= Path.Combine(Path.GetTempPath(), "CodeAnalysis", "OmnisharpAnalyzerShadowCopies");
return DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader(baseDirectory);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System;
using System.Composition;
using System.IO;
using Microsoft.CodeAnalysis.Host.Mef;

namespace Microsoft.CodeAnalysis.Host
Expand All @@ -12,8 +13,9 @@ namespace Microsoft.CodeAnalysis.Host
[ExportWorkspaceService(typeof(IAnalyzerAssemblyLoaderProvider))]
internal sealed class DefaultAnalyzerAssemblyLoaderService : IAnalyzerAssemblyLoaderProvider
{
private readonly DefaultAnalyzerAssemblyLoader _loader = new();
private readonly ShadowCopyAnalyzerAssemblyLoader _shadowCopyLoader = new();
private readonly IAnalyzerAssemblyLoader _loader = new DefaultAnalyzerAssemblyLoader();
private readonly IAnalyzerAssemblyLoader _shadowCopyLoader = DefaultAnalyzerAssemblyLoader.CreateNonLockingLoader(
Path.Combine(Path.GetTempPath(), "CodeAnalysis", "WorkspacesAnalyzerShadowCopies"));

[ImportingConstructor]
[Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ internal sealed class RemoteAnalyzerAssemblyLoader : AnalyzerAssemblyLoader

public RemoteAnalyzerAssemblyLoader(string baseDirectory)
{
// jason should we load from stream here on Linux or is this VS only?
_baseDirectory = baseDirectory;
}

Expand Down

0 comments on commit f7cb1f2

Please sign in to comment.