Remove weak references in skeleton reference cache. by CyrusNajmabadi · Pull Request #61608 · dotnet/roslyn

CyrusNajmabadi · 2022-05-31T21:22:00Z

Traces taken indicate we're consume lots of resources generating skeleton references in OOP. Auditing the code i found the use of weak-references to hold onto skeletons highly suspect. The presumption should be that if we've computed the skeleton for a compilation because a downstream project needs it, then we'll almost certainly continue to need that skeleton in the future as the downstream project changes. We'd only want to actually let it get cleaned up when the compilatino-tracker owning hte skeleton actually goes away.

CyrusNajmabadi · 2022-05-31T21:36:27Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

-            /// calls can early exit.
+            /// The actual assembly metadata produced from the data pointed to in <see cref="_storage"/>.
+            /// </summary>
+            private readonly AsyncLazy<AssemblyMetadata?> _metadata;


this is a change from before. previously we might create several instances of AssemblyMetadta if the same compilation was asked for multiple MetadataReferences to it (with different MetadataReferenceProperties). now, you'll get the same AssemblyMetadata that will be reused in all those cases.

This supports the compiler docs which explicitly state:

/// <see cref="Metadata"/> objects may cache information decoded from the PE image. /// Reusing <see cref="Metadata"/> instances across metadata references will result in better performance.

CyrusNajmabadi · 2022-05-31T21:38:24Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

            /// This instance should be locked when being read/written.
            /// </remarks>
-            private readonly Dictionary<MetadataReferenceProperties, WeakReference<MetadataReference>?> _metadataReferences = new();
+            private readonly Dictionary<MetadataReferenceProperties, AsyncLazy<MetadataReference?>> _metadataReferences = new();


same as before, but no longer weak. As per the OP of this PR, having htis be weak seems like an antipattern.

hmmm.. if weak reference is going away, I think that means the primaryWorkspace in OOP is having problem (not stay alive) or ftemporaryWorkspace forked from primaryWorkspace is not getting skeleton assembly as expected and re-creating them. or property instance is not reused properly?

but I guess with 64bits, holding some skeleton assemblies in memory a bit longer won't matter much now.

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

tmat · 2022-05-31T22:47:20Z

                // it would be great if compiler can be little bit smarter on how it deals with stream.

Do you know what this comment means? What should the compiler do better?

Refers to: src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs:311 in de7b4c5. [](commit_id = de7b4c5, deletion_comment = False)

tmat · 2022-05-31T22:48:46Z

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

                    // Tie lifetime of stream to metadata we created. It is important to tie this to the Metadata and not the
                    // metadata reference, as PE symbols hold onto just the Metadata. We can use Add here since we created
                    // a brand new object in AssemblyMetadata.Create above.
                    s_lifetime.Add(metadata, supportNativeMemory);


s_lifetime

Since we now hold on the metadata in AsyncLazy, do we actually need this?

We do yes, and it's vital (fortunately tests will crash). The reason for this is that metadata-references are handed out and can effectively live longer than our own internal data structures. One way to imagine this is simply that we handle out compilations through our Project API, but the user then lets go of the Solution instance (or it moves forward on the workspace). All the existing snapshots in the Solution/Project/CompolationTracker snapshot get GC'ed and explosions happen.

Effectively, we're saying as long as anything is referencing this metadata, this stream cannot be removed.

--

Note: i learned this from actually trying to remove it and discovering that yes it does blow up :)

I see, so alternatively we could implement a subtype of PortableExecutableReference, return an instance of it and store the memory object to it.

I wonder if it would be best to add ModuleMetadata.CreateFromImage() overload that takes an object memoryOwner. That way whenever you create a pointer based module metadata you can just pass in the object that keeps it alive and not worry about it.

I see, so alternatively we could implement a subtype of PortableExecutableReference, return an instance of it and store the memory object to it.

Yes. That would work. But there's already a subclass being returned. Slightly worried about having to preserve semantics with that. However, i am very open to this as i hate CWTs and would love to get rid of that, so let me try that.

I wonder if it would be best to add ModuleMetadata.CreateFromImage() overload that takes an object memoryOwner. That way whenever you create a pointer based module metadata you can just pass in the object that keeps it alive and not worry about it.

That would be a way to do it :)

ok. i started down teh compiler side, and it was a lot of changes to the public API. I'll do that in a followup. FOr now though i made a custom subclass where we can hold onto the memory ourselves.

CyrusNajmabadi · 2022-05-31T22:59:18Z

Do you know what this comment means? What should the compiler do better?

I don't know. I think @heejaechang may have written the comment. He might know :)

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs

…SkeletonReferenceCache.cs

jasonmalinowski · 2022-05-31T23:23:31Z

src/VisualStudio/Core/Def/Remote/VisualStudioWorkspaceServiceHubConnector.cs

        public void StopListening(Workspace workspace)
        {
-            if (!(workspace is VisualStudioWorkspace) || IVsShellExtensions.IsInCommandLineMode(_threadingContext.JoinableTaskFactory))
+            if (workspace is not VisualStudioWorkspace || IVsShellExtensions.IsInCommandLineMode(_threadingContext.JoinableTaskFactory))


Change is fine, not sure if you meant it to be in this PR though?

heejaechang · 2022-06-01T00:49:19Z

Hi there :)

Do you know what this comment means? What should the compiler do better?

It refers to comments right above - "if we give stream, compiler will internally copy it to native memory again",

it means it would be nice if compiler (by compiler, roslyn's compiler code) know when to copy or reuse given stream so we don't need to have this in IDE side. and all we need to do is just AssemblyMetadata.CreateFromStream and compiler side does everything in the most efficient way and deal with lifetime themselves in case of re-using native memory.

CyrusNajmabadi added 2 commits May 31, 2022 12:42

Simlpify

1ad7861

Remove weak ref

49813db

CyrusNajmabadi requested a review from jasonmalinowski May 31, 2022 21:22

ghost added the Area-IDE label May 31, 2022

CyrusNajmabadi added 2 commits May 31, 2022 14:24

Add comment

9fec72c

Simplify further

de7b4c5

CyrusNajmabadi commented May 31, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs Outdated Show resolved Hide resolved

CyrusNajmabadi marked this pull request as ready for review May 31, 2022 21:39

CyrusNajmabadi requested a review from a team as a code owner May 31, 2022 21:39

tmat reviewed May 31, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs Outdated Show resolved Hide resolved

tmat reviewed May 31, 2022

View reviewed changes

Update docs

414606d

tmat reviewed May 31, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs Outdated Show resolved Hide resolved

sharwell reviewed May 31, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.SkeletonReferenceCache.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 2 commits May 31, 2022 16:15

Update src/Workspaces/Core/Portable/Workspace/Solution/SolutionState.…

254d1c3

…SkeletonReferenceCache.cs

Simplify

98a424e

CyrusNajmabadi requested review from sharwell and tmat May 31, 2022 23:22

CyrusNajmabadi added 2 commits May 31, 2022 16:25

Simplify

083c1c1

Proper nrt

1bf000c

jasonmalinowski approved these changes May 31, 2022

View reviewed changes

nrt

7503ac2

CyrusNajmabadi added 2 commits May 31, 2022 18:17

Provide impl of PERef so we can hold strong reference to memory

e804abd

Share the same object

83f273c

CyrusNajmabadi enabled auto-merge June 1, 2022 15:12

CyrusNajmabadi mentioned this pull request Jun 1, 2022

Update compiler to supported UnmanagedMemoryStream when creating ModuleMetadata. #61630

Merged

CyrusNajmabadi merged commit c025a20 into dotnet:main Jun 1, 2022

ghost added this to the Next milestone Jun 1, 2022

CyrusNajmabadi deleted the solutionSync branch June 1, 2022 16:48

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

Conversation

CyrusNajmabadi commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmat commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented May 31, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heejaechang commented Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

heejaechang Jun 1, 2022 •

edited

Loading

heejaechang commented Jun 1, 2022 •

edited

Loading