Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. #62373
Conversation
| @@ -76,7 +76,7 @@ override sealed Microsoft.CodeAnalysis.CompilationOptions.GetHashCode() -> int | |||
| override sealed Microsoft.CodeAnalysis.Diagnostic.Equals(object? obj) -> bool | |||
| *REMOVED*static Microsoft.CodeAnalysis.SyntaxNodeExtensions.ReplaceSyntax<TRoot>(this TRoot! root, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxNode!>! nodes, System.Func<Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!>! computeReplacementNode, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxToken>! tokens, System.Func<Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken>! computeReplacementToken, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxTrivia>! trivia, System.Func<Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia>! computeReplacementTrivia) -> TRoot! | |||
| static Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation.Create(System.Collections.Immutable.ImmutableArray<byte> compressedSlotMap, System.Collections.Immutable.ImmutableArray<byte> compressedLambdaMap, System.Collections.Immutable.ImmutableArray<byte> compressedStateMachineStateMap) -> Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation | |||
| static Microsoft.CodeAnalysis.ModuleMetadata.CreateFromMetadata(System.IntPtr metadata, int size, System.IDisposable! owner, bool disposeOwner) -> Microsoft.CodeAnalysis.ModuleMetadata! | |||
| static Microsoft.CodeAnalysis.ModuleMetadata.CreateFromMetadata(System.IntPtr metadata, int size, System.Action! onDispose) -> Microsoft.CodeAnalysis.ModuleMetadata! | |||
There was a problem hiding this comment.
I like this new public API. Makes me happy :).
| /// Whether or not to call _onDispose when this is disposed. The reason that this is controlled by a flag is | ||
| /// that if we make a copy of this modulemetadata, we still want to keep the Action alive in it (as it may be | ||
| /// keeping alive underlying object this metadata needs). However, the copy of the metadata should not call the | ||
| /// action as only the <see cref="Metadata.IsImageOwner"/> metadata is responsible for that. |
There was a problem hiding this comment.
This seems dangerous to me. Who says that the copy has a shorter lifetime than the original? Feels like we need some sort of reference count wrapper that is decremented on dispose of the containing reference.
There was a problem hiding this comment.
i agree ti's dangerous. BUt it's already the established semantics of ModuleMetadta. e.g. prior to this change if you did this:
ModuleMetadata mm = ...
mmCopy = mm.Copy();
mm.Dispose();then mmCopy was now in an 'no longer usable' state (same with any other copies created). I'm genuinely surprised we went with those semantics, but it's been the way of this type since the beginning, so it's just part of the nature of these and hte entire stack is written to be ok with that.
I would totally be ok with moving to a refcounted system. And, importantly, it would not change this new API at all. All it would do would be to allow us to remove those comments and make things safer. As such, i would prefer it be a separate orthogonal API :)
There was a problem hiding this comment.
I'm genuinely surprised we went with those semantics,
For performance reasons. Entirely safe semantics would be slow since every read from the metadata would need to check whether the memory is disposed or not before it reads and do these two operations atomically.
There was a problem hiding this comment.
Who says that the copy has a shorter lifetime than the original?
It's responsibility of the owner of the metadata to guarantee that.
There was a problem hiding this comment.
It would probably be possible to make metadata reads safe using ReadOnlySpan<T> and ReadOnlyMemory<T>. But that would require a lot of changes throughout SRM and also might not be sufficiently efficient on .NET Framework.
There was a problem hiding this comment.
I guess making the copies ref-counted would add some value. It wouldn't make it entirely safe but perhaps safer.
|
@dotnet/roslyn-compiler for eyes. Thanks! |
|
@333fred @dotnet/roslyn-compiler ptal. thanks! |
| /// Disposed. Is controlled by the <c>leaveOpen</c> flag in <see cref="CreateFromStream(Stream, bool)"/>, or | ||
| /// the <see cref="PEStreamOptions.LeaveOpen"/> flag in <see cref="CreateFromStream(Stream, PEStreamOptions)"/>. | ||
| /// Whether or not to call _onDispose when this is disposed. The reason that this is controlled by a flag is | ||
| /// that if we make a copy of this modulemetadata, we still want to keep the Action alive in it (as it may be |
src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs
Outdated
Show resolved
Hide resolved
src/Compilers/Core/Portable/MetadataReference/ModuleMetadata.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the 
* upstream/main: (62 commits) Prevent assert from being hit (dotnet#62366) Don't offer '??=' for pointer types (dotnet#62476) Integrate generator times into /reportAnalyzer (dotnet#61661) Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373) Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444) Update PublishData.json Complete 'file' support for `SyntaxGenerator` (dotnet#62413) Apply changes directly to text buffer (dotnet#62337) Remove LangVer check from extended nameof binding (dotnet#62339) Fixed shared project file error (dotnet#62466) Handle new error codes Use MSBuid generated property for package path Exclude BCL libraries from Roslyn vsix Bump the integration test timeouts a bit Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322) Test helpers shouldn't escape quotes by default (dotnet#62321) Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320) Install 3.1 runtime for SBOM tool Generate VS SBOM during official build. Minor refactoring in 'required' handling for override completion (dotnet#62422) ...
Fixes #61778
PRs #61630 and #61693 added support to the compiler metadata components to allow them to hold onto an object (specifically an IDisposable) during their lifetime that they would then dispose when teh actual metadata component was disposed.
This was very beneficial for hte IDE as the IDE often uses APIs that allocate and pin unmanaged memory which should be cleaned up when no longer needed. Prior to these APIs, tehre was lots of codein the IDE (including several CWTs) that were used to ensure this, though with high complexity. The ability to have hte metadata itself just take over this responsibility greatly simplified things.
However, during API review, it was felt that hte API was a bit too complex and transparent. It requires two parameters to be passed in, and it makes it requires that the main parameter be an IDisposable object. This is really not necessary as all we actually need to support this concept is just a way to know when teh metadata itself is being disposed. What is done with that knowledge doesn't need to be encoded into the contract here.
To that end, this PR changes the API in question to be a much simpler one where instead of passing in two low-level values into the API, an opaque
Actionis passed in instead. When the metadata is disposed, thatActionwill be invoked, allowing the consumer to determine what should happen in that case.The end result is functionally the same, but it's a simpler and cleaner API choice.