Do not use collectible AssemblyLoadContexts in AnalyzerAssemblyLoader. #79990
Conversation
We ship several analyzers and source-generators in the SDK that are added to most projects. We want to ship these as Ready2Run so that we reduce JIT time. However, when an assembly is loaded into a collectible AssemblyLoadContext it prevents any of the R2R logic from being used.
| [CombinatorialData] | ||
| public void AssemblyLoading_DoesNotUseCollectibleALCs(AnalyzerTestKind kind) | ||
| { | ||
| // When an assembly is loaded into a collectible AssemblyLoadContext it prevents any of the R2R logic from being used. |
There was a problem hiding this comment.
Put a blurb that this R2R is critical to our VS / CLI perfromance scenarios. Basically a place for some future dev to look for context if they ever want to change this.
|
I'm curious, how does this affect generator/analyzer reloading in VS? I assumed we were unloading the old ALCs when we load new ones, but maybe not? Are we just going to keep loading things into memory until you restart? That's probably fine as it'll just get paged out and never used again but just want to understand if so. |
| try | ||
| { | ||
| context.Unload(); | ||
| CodeAnalysisEventSource.Log.DisposeAssemblyLoadContext(context.Directory, context.ToString()); |
There was a problem hiding this comment.
I waffled on whether to remove these EventSource methods. I see that they are numbered and didn't want to throw off any telemetry. Would marking them as obsolete be the way to go?
There was a problem hiding this comment.
Seems like a reasonable approach.
There was a problem hiding this comment.
I'll do this in a follow up.
|
|
||
| public DirectoryLoadContext(string directory, AnalyzerAssemblyLoader loader) | ||
| : base(isCollectible: true) | ||
| : base(isCollectible: false) |
There was a problem hiding this comment.
@CyrusNajmabadi I saw that Extensions use the assembly loader. Will this break those scenarios?
There was a problem hiding this comment.
The extension assembly support is supposed to be able to hot unload extensions. We definitely are calling it here here -
In this, are the assemblies still getting unloaded, but we're not disposing of the ALC? Or are both the assemblies and ALC staying around?
There was a problem hiding this comment.
Going to take this as is but will follow up with a PR making isCollectible configurable.
There was a problem hiding this comment.
yeah. we should make this configurable.
That matches my understanding. As the old IsolatedAnalyzerReferenceSets get finalized they dispose their AnalyzerAssemblyLoader which will no longer unload. |
| try | ||
| { | ||
| context.Unload(); | ||
| CodeAnalysisEventSource.Log.DisposeAssemblyLoadContext(context.Directory, context.ToString()); |
There was a problem hiding this comment.
Seems like a reasonable approach.
We ship several analyzers and source-generators in the SDK that are added to most projects. We want to ship these as Ready2Run so that we reduce JIT time. However, when an assembly is loaded into a collectible AssemblyLoadContext it prevents any of the R2R logic from being used.