Allow AnalyzerAssemblyLoader to be collectible #79997
Conversation
Some scenario where AnalyzerAssemblyLoader was useds, such as Extensions, relied on the ALC to be collectible.
| ImmutableArray<IAnalyzerAssemblyResolver> assemblyResolvers = default, | ||
| System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null) | ||
| System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null, | ||
| bool isCollectible = false) |
There was a problem hiding this comment.
The name isCollectible is not quite sitting right with me. The behavior it's controlling is what happens on dispose vs. the instance itself being collectible. Maybe collectOnDispose? @jjonescz may have suggestions.
There was a problem hiding this comment.
collectOnDispose sounds good to me
| ImmutableArray<IAnalyzerAssemblyResolver> assemblyResolvers = default, | ||
| System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null) | ||
| System.Runtime.Loader.AssemblyLoadContext? compilerLoadContext = null, | ||
| bool isCollectible = false) |
There was a problem hiding this comment.
collectOnDispose sounds good to me
chsienki
left a comment
chsienki
left a comment
There was a problem hiding this comment.
LGTM, a moral approve as I don't count as a Compiler sign off anymore.
| // to bubble out as it must be fixed. | ||
| var analyzerAssemblyLoaderProvider = _workspaceServices.GetRequiredService<IAnalyzerAssemblyLoaderProvider>(); | ||
| var analyzerAssemblyLoader = analyzerAssemblyLoaderProvider.CreateNewShadowCopyLoader(); | ||
| var analyzerAssemblyLoader = analyzerAssemblyLoaderProvider.CreateNewShadowCopyLoader(collectOnDispose: true); |
There was a problem hiding this comment.
was there another place where we need to set this to true, e.g. for analyzers and generators in the OOP process? cc @CyrusNajmabadi
There was a problem hiding this comment.
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?
| /// cref="ISourceGenerator"/>s in a fresh <see cref="AssemblyLoadContext"/>. | ||
| /// </summary> | ||
| IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader(); | ||
| IAnalyzerAssemblyLoaderInternal CreateNewShadowCopyLoader() => CreateNewShadowCopyLoader(collectOnDispose: false); |
There was a problem hiding this comment.
this seems undesirable for the workspace layer. can we make this either collectOnDispose: true, or have this not be optional so that we are forced to decide what to do with every caller.
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
I don't like the defualt, as it makes it too easy for the IDE to potentially do the wrong thing.
|
|
||
| internal AnalyzerAssemblyLoader(ImmutableArray<IAnalyzerPathResolver> pathResolvers) | ||
| : this(pathResolvers, assemblyResolvers: [DiskAnalyzerAssemblyResolver], compilerLoadContext: null) | ||
| : this(pathResolvers, assemblyResolvers: [DiskAnalyzerAssemblyResolver], compilerLoadContext: null, collectOnDispose: false) |
There was a problem hiding this comment.
Don't we want to collect here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Some scenario where AnalyzerAssemblyLoader was useds, such as Extensions, relied on the ALC to be collectible.