Remove solution replicatoin context #73117
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| if (_storage is not null) | ||
| { | ||
| context.AddResource(_storage); |
There was a problem hiding this comment.
the underlying _storage instance for a SerializableSourceText is never disposed. So there's no way for hte memory it points to to go away. So there's no need to root it in this context object.
| @@ -331,8 +329,6 @@ private static void WriteTo(Metadata? metadata, ObjectWriter writer, Cancellatio | |||
| return false; | |||
| } | |||
|
|
|||
| context.AddResource(storage); | |||
There was a problem hiding this comment.
this is a similar issue. walking through all the ITempStorage code, none of the types that we are serializing here which dump themselves into temp-storage ever get disposed. so there's no need to keep this alive either.
| // PinnedObject will be kept alive as long as the ModuleMetadata is alive due to passing its .Dispose method in | ||
| // as the onDispose callback of the metadata. | ||
| return ModuleMetadata.CreateFromMetadata( | ||
| pinnedObject.GetPointer(), (int)length, pinnedObject.Dispose); | ||
| } | ||
|
|
||
| private static void CopyByteArrayToStream(ObjectReader reader, Stream stream, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Note: look into the deleted SolutionReplicationContext file below.
| // Currently we don't dispose resources, only keep them alive. | ||
| // Shouldn't we dispose them? | ||
| // _resources.All(resource => resource.Dispose()); | ||
| s_pool.ClearAndFree(_resources); |
There was a problem hiding this comment.
note how we never dispose tehse objects. Nor should we. When serializing out to oop, just because something was written out to a MMF does not mean we should dispose that mmf, it's an entirely orthogonal concern outside of the scope of a remote call to oop.
The only value this type produced was to root those disposables, presumably to ensure that anything they were holding onto didn't go away over the lifetime of hte call. However, none of that is necessary. Either we're literally serializing out a value by-value over the wire. In which case we don't need to keep it alive. Or we're serializing it out by-reference (e.g. serializing out it's location in an MMF). But in all those cases, the mmfs are never released anyways, so there's no need to keep anything alive.
|
@ToddGrun this is ready for review. I've now audited everything in the MMF codebase. And we definitely do not ever release the data. so all the work to try to keep objects rooted is just unnecessary and can be removed. |
|
OK, I think this looks good, the explanations definitely helped. In reply to: 2067542936 |
|
@jasonmalinowski For review when you get back. |
I'm virtually certain this type serves no purpose. I think the original intent was to root objects in memory so that they wouldn't get cleaned up, causing their underlying memory to go away, while we were in the process of communicating with our oop process.
However, what has become clearer over investigation, and several refactorings, is that our snapshots basically do not get dropped (they are dumped into memory mapped files that stay around). As such, needing to root anything isn't necessary.