Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1646,19 +1646,19 @@ public void AssemblyLoading_ResourcesInParent(AnalyzerTestKind kind)

[Theory]
[CombinatorialData]
public void AssemblyLoadingInNonDefaultContext_AnalyzerReferencesSystemCollectionsImmutable(AnalyzerTestKind kind)
public void AssemblyLoadingInNonDefaultContext_AnalyzerReferencesSystemCollectionsImmutable(AnalyzerTestKind kind, bool collectOnDispose)
{
// Load the compiler assembly and a modified version of S.C.I into the compiler load context. We
// expect the analyzer will use the bogus S.C.I in the compiler context instead of the one
// in the host context.
var alc = new AssemblyLoadContext(nameof(AssemblyResolver_FirstOneWins), isCollectible: false);
var alc = new AssemblyLoadContext(nameof(AssemblyResolver_FirstOneWins), collectOnDispose);
_ = alc.LoadFromAssemblyPath(TestFixture.UserSystemCollectionsImmutable);
_ = alc.LoadFromAssemblyPath(typeof(AnalyzerAssemblyLoader).GetTypeInfo().Assembly.Location);
var loader = kind switch
{
AnalyzerTestKind.LoadStream => new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.StreamAnalyzerAssemblyResolver], alc),
AnalyzerTestKind.LoadDirect => new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], alc),
AnalyzerTestKind.ShadowLoad => new AnalyzerAssemblyLoader([new ShadowCopyAnalyzerPathResolver(Temp.CreateDirectory().Path)], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], alc),
AnalyzerTestKind.LoadStream => new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.StreamAnalyzerAssemblyResolver], alc, collectOnDispose),
AnalyzerTestKind.LoadDirect => new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], alc, collectOnDispose),
AnalyzerTestKind.ShadowLoad => new AnalyzerAssemblyLoader([new ShadowCopyAnalyzerPathResolver(Temp.CreateDirectory().Path)], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], alc, collectOnDispose),
_ => throw ExceptionUtilities.UnexpectedValue(kind)
};

Expand All @@ -1675,6 +1675,11 @@ public void AssemblyLoadingInNonDefaultContext_AnalyzerReferencesSystemCollectio

Assert.Equal("42", sb.ToString());
});

if (collectOnDispose)
{
alc.Unload();
}
}

[Theory]
Expand Down Expand Up @@ -1787,7 +1792,7 @@ public void AssemblyResolver_FirstOneWins()
var resolver1 = new TestAnalyzerAssemblyResolver((_, assemblyName, current, _) =>
assemblyName.Name == name ? current.LoadFromAssemblyPath(TestFixture.Delta1) : null);
var resolver2 = new TestAnalyzerAssemblyResolver((_, _, assemblyName, _) => null);
var loader = new AnalyzerAssemblyLoader([], [resolver1, resolver2], alc);
var loader = new AnalyzerAssemblyLoader([], [resolver1, resolver2], alc, collectOnDispose: false);

Run(loader, state: name, static (AnalyzerAssemblyLoader loader, AssemblyLoadTestFixture testFixture, object state) =>
{
Expand All @@ -1809,7 +1814,7 @@ public void AssemblyResolver_ThrowOnNoMatch()
var name = Path.GetFileNameWithoutExtension(TestFixture.Delta1);
var alc = new AssemblyLoadContext(nameof(AssemblyResolver_FirstOneWins), isCollectible: true);
var resolver = new TestAnalyzerAssemblyResolver((_, _, assemblyName, _) => null);
var loader = new AnalyzerAssemblyLoader([], [resolver], alc);
var loader = new AnalyzerAssemblyLoader([], [resolver], alc, collectOnDispose: false);

Run(loader, static (AnalyzerAssemblyLoader loader, AssemblyLoadTestFixture testFixture) =>
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public CompilerResolverTests()
CompilerContext = new AssemblyLoadContext(nameof(CompilerResolverTests), isCollectible: true);
AssemblyInCompilerContext = CompilerContext.LoadFromAssemblyPath(typeof(AnalyzerAssemblyLoader).Assembly.Location);
ScratchContext = new AssemblyLoadContext("Scratch", isCollectible: true);
Loader = new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], CompilerContext);
Loader = new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], CompilerContext, collectOnDispose: false);
}

public void Dispose()
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/CodeAnalysisTest/InvokeUtil.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ internal void Exec(
throw ExceptionUtilities.Unreachable();
}

var loader = new AnalyzerAssemblyLoader(pathResolvers, assemblyResolvers, compilerLoadContext: null);
var loader = new AnalyzerAssemblyLoader(pathResolvers, assemblyResolvers, compilerLoadContext: null, collectOnDispose: false);
var compilerContextAssemblies = loader.CompilerLoadContext.Assemblies.SelectAsArray(a => a.FullName);
try
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,15 @@ internal sealed partial class AnalyzerAssemblyLoader
public IAnalyzerAssemblyResolver CompilerAnalyzerAssemblyResolver { get; }
public AssemblyLoadContext CompilerLoadContext { get; }
public ImmutableArray<IAnalyzerAssemblyResolver> AnalyzerAssemblyResolvers { get; }
public bool CollectOnDispose { get; }

internal AnalyzerAssemblyLoader()
: this(pathResolvers: [])
{
}

internal AnalyzerAssemblyLoader(ImmutableArray<IAnalyzerPathResolver> pathResolvers)
: this(pathResolvers, assemblyResolvers: [DiskAnalyzerAssemblyResolver], compilerLoadContext: null)
: this(pathResolvers, assemblyResolvers: [DiskAnalyzerAssemblyResolver], compilerLoadContext: null, collectOnDispose: false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to collect here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe collecting will free memory, assuming nothing has somehow pinned the ALC. But collecting will definitely disable R2R leading to performance issues down the road as more analyzers and generators shipped in the SDK are crossgen'd.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is R2R and why does collecting cause a payment for them.

The purpose of us making this collectible is for the important scenario of someone developing their own generators/analyzers and continually recompiling them and wanting them reloaded in OOP without having to restart VS.

{
}

Expand All @@ -53,13 +54,15 @@ internal AnalyzerAssemblyLoader(ImmutableArray<IAnalyzerPathResolver> pathResolv
internal AnalyzerAssemblyLoader(
ImmutableArray<IAnalyzerPathResolver> pathResolvers,
ImmutableArray<IAnalyzerAssemblyResolver> assemblyResolvers,
AssemblyLoadContext? compilerLoadContext)
AssemblyLoadContext? compilerLoadContext,
bool collectOnDispose)
{
if (assemblyResolvers.Length == 0)
{
throw new ArgumentException("Cannot be empty", nameof(assemblyResolvers));
}

CollectOnDispose = collectOnDispose;
CompilerLoadContext = compilerLoadContext ?? AssemblyLoadContext.GetLoadContext(typeof(SyntaxTree).GetTypeInfo().Assembly)!;
CompilerAnalyzerAssemblyResolver = new CompilerResolver(CompilerLoadContext);
AnalyzerPathResolvers = pathResolvers;
Expand Down Expand Up @@ -165,9 +168,45 @@ internal DirectoryLoadContext[] GetDirectoryLoadContextsSnapshot()

private partial void DisposeWorker()
{
lock (_guard)
if (CollectOnDispose)
{
disposeCollectibleWorker();
}
else
{
lock (_guard)
{
_loadContextByDirectory.Clear();
}
}

return;

void disposeCollectibleWorker()
{
_loadContextByDirectory.Clear();
var contexts = ArrayBuilder<DirectoryLoadContext>.GetInstance();
lock (_guard)
{
foreach (var (_, context) in _loadContextByDirectory)
contexts.Add(context);

_loadContextByDirectory.Clear();
}

foreach (var context in contexts)
{
try
{
context.Unload();
CodeAnalysisEventSource.Log.DisposeAssemblyLoadContext(context.Directory, context.ToString());
}
catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Critical))
{
CodeAnalysisEventSource.Log.DisposeAssemblyLoadContextException(context.Directory, ex.ToString(), context.ToString());
}
}

contexts.Free();
}
}

Expand All @@ -177,7 +216,7 @@ internal sealed class DirectoryLoadContext : AssemblyLoadContext
private readonly AnalyzerAssemblyLoader _loader;

public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader)
: base(isCollectible: false)
: base(isCollectible: loader.CollectOnDispose)
{
Directory = directory;
_loader = loader;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,8 @@ internal static IAnalyzerAssemblyLoaderInternal CreateNonLockingLoader(
string windowsShadowPath,
ImmutableArray<IAnalyzerPathResolver> pathResolvers = default,
ImmutableArray<IAnalyzerAssemblyResolver> assemblyResolvers = default,
System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null)
System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null,
bool collectOnDispose = false)
{
CodeAnalysisEventSource.Log.CreateNonLockingLoader(windowsShadowPath);
pathResolvers = pathResolvers.NullToEmpty();
Expand All @@ -424,7 +425,8 @@ internal static IAnalyzerAssemblyLoaderInternal CreateNonLockingLoader(
return new AnalyzerAssemblyLoader(
pathResolvers,
[.. assemblyResolvers, StreamResolver.Instance],
compilerLoadContext);
compilerLoadContext,
collectOnDispose);
}

// The goal here is to avoid locking files on disk that are reasonably expected to be changed by
Expand All @@ -434,7 +436,8 @@ internal static IAnalyzerAssemblyLoaderInternal CreateNonLockingLoader(
return new AnalyzerAssemblyLoader(
[.. pathResolvers, ProgramFilesAnalyzerPathResolver.Instance, new ShadowCopyAnalyzerPathResolver(windowsShadowPath)],
[.. assemblyResolvers, DiskResolver.Instance],
compilerLoadContext);
compilerLoadContext,
collectOnDispose);
}

#else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ private sealed class DefaultExtensionAssemblyLoaderProvider(HostWorkspaceService
// These lines should always succeed. If they don't, they indicate a bug in our code that we want
// to bubble out as it must be fixed.
var analyzerAssemblyLoaderProvider = _workspaceServices.GetRequiredService<IAnalyzerAssemblyLoaderProvider>();
var analyzerAssemblyLoader = analyzerAssemblyLoaderProvider.CreateNewShadowCopyLoader();
var analyzerAssemblyLoader = analyzerAssemblyLoaderProvider.CreateNewShadowCopyLoader(collectOnDispose: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was there another place where we need to set this to true, e.g. for analyzers and generators in the OOP process? cc @CyrusNajmabadi

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can imagine we would want those analyzers and generators to run with R2R when possible. Also, are we certain that these assemblies are ever actually unloaded?


// Catch exceptions here related to working with the file system. If we can't properly enumerate,
// we want to report that back to the client, while not blocking the entire extension service.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public ImmutableArray<AnalyzerFileReference> CreateSolutionLevelAnalyzerReferenc
// Load all analyzers into a fresh shadow copied load context. In the future, if we want to support reloading
// of solution-level analyzer references, we should just need to listen for changes to those analyzer paths and
// then call back into this method to update the solution accordingly.
var analyzerLoader = loaderProvider.CreateNewShadowCopyLoader();
var analyzerLoader = loaderProvider.CreateNewShadowCopyLoader(collectOnDispose: false);

foreach (var analyzerPath in _solutionLevelAnalyzerPaths)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@
using Basic.Reference.Assemblies;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Test.Utilities;
using Roslyn.Utilities;
using Roslyn.Test.Utilities;
using Roslyn.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.ExternalAccess.Razor.UnitTests;
Expand Down Expand Up @@ -76,7 +76,7 @@ private static void RunWithLoader(Action<RazorAnalyzerAssemblyResolver, Analyzer
{
var compilerLoadContext = new AssemblyLoadContext("Compiler", isCollectible: true);
var currentLoadContext = new AssemblyLoadContext("Current", isCollectible: true);
var loader = new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], compilerLoadContext);
var loader = new AnalyzerAssemblyLoader([], [AnalyzerAssemblyLoader.DiskAnalyzerAssemblyResolver], compilerLoadContext, collectOnDispose: false);
#pragma warning disable 612
var resolver = CreateResolver();
#pragma warning restore 612
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ internal interface IAnalyzerAssemblyLoaderProvider : IWorkspaceService
/// Creates a fresh shadow copying loader that will load all <see cref="AnalyzerReference"/>s and <see
/// cref="ISourceGenerator"/>s in a fresh <see cref="AssemblyLoadContext"/>.
/// </summary>
IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader();
IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader(bool collectOnDispose);
#endif
}

Expand All @@ -43,18 +43,20 @@ internal abstract class AbstractAnalyzerAssemblyLoaderProvider : IAnalyzerAssemb
public AbstractAnalyzerAssemblyLoaderProvider(IEnumerable<IAnalyzerAssemblyResolver> assemblyResolvers, IEnumerable<IAnalyzerPathResolver> assemblyPathResolvers)
{
_assemblyResolvers = [.. assemblyResolvers];
_shadowCopyLoader = new(CreateNewShadowCopyLoader);
_shadowCopyLoader = new(CreateNewShadowCopyLoader(isCollectible: false));
_assemblyPathResolvers = [.. assemblyPathResolvers];
}

public IAnalyzerAssemblyLoaderInternal SharedShadowCopyLoader
=> _shadowCopyLoader.Value;

public IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader()
public IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader(bool isCollectible)
=> this.WrapLoader(AnalyzerAssemblyLoader.CreateNonLockingLoader(
Path.Combine(Path.GetTempPath(), nameof(Roslyn), "AnalyzerAssemblyLoader"),
_assemblyPathResolvers,
_assemblyResolvers));
_assemblyResolvers,
compilerLoadContext: null,
isCollectible));
#else
private readonly Lazy<IAnalyzerAssemblyLoaderInternal> _shadowCopyLoader;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ private IsolatedAnalyzerReferenceSet(
IAnalyzerAssemblyLoaderProvider provider)
{
// Make a fresh loader that uses that ALC that will ensure these references are properly isolated.
_shadowCopyLoader = provider.CreateNewShadowCopyLoader();
_shadowCopyLoader = provider.CreateNewShadowCopyLoader(collectOnDispose: false);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public sealed class RemoteAnalyzerAssemblyLoaderTests
private static AnalyzerAssemblyLoader Create(string baseDirectory) => new(
[new RemoteAnalyzerPathResolver(baseDirectory)],
[AnalyzerAssemblyLoader.StreamAnalyzerAssemblyResolver],
compilerLoadContext: null);
compilerLoadContext: null,
collectOnDispose: false);

[Fact]
public void NonIdeAnalyzerAssemblyShouldBeLoadedInSeparateALC()
Expand Down
Loading