Skip to content

Cache the AssemblyMetadata produced for any given compilation#61649

Merged
CyrusNajmabadi merged 6 commits intodotnet:mainfrom
CyrusNajmabadi:cacheSkeletonSet
Jun 3, 2022
Merged

Cache the AssemblyMetadata produced for any given compilation#61649
CyrusNajmabadi merged 6 commits intodotnet:mainfrom
CyrusNajmabadi:cacheSkeletonSet

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

This ensures that if we produce an AssemblyMetadata (and skeleton PEReference) to be consumed a downstream project, that we can then reuse that same data for a different downstream project, even if the first downstream project is GC'ed.

As long as the originating compilation is around, then we'll only build a skeleton for it at most once. This also benefits any scenarios that may exist where we might get a forked compilation-tracker but that tracker ends up producing the same compilation (@jasonmalinowski for if this is possible).

This will also get cleaner once #61630 goes in.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner June 2, 2022 00:07
@ghost ghost added the Area-IDE label Jun 2, 2022
/// want to keep it's skeleton cache around.
/// </summary>
private readonly SemaphoreSlim _emitGate = new(initialCount: 1);
private static readonly ConditionalWeakTable<Compilation, AsyncLazy<SkeletonReferenceSet?>> s_compilationToSkeletonSet = new();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for the gate. the CWT is already thread-safe on access, and the use of AsyncLazy means there's at most one computation ever happening to produce the set.

var lazy = s_compilationToSkeletonSet.GetValue(compilation,
compilation => new AsyncLazy<SkeletonReferenceSet?>(
cancellationToken => Task.FromResult(CreateSkeletonSet(workspace, compilation, cancellationToken)),
cacheResult: true));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would love these to be static lambdas. However, we are keyed off the compilation, but need to capture 'workspace' to create the result.

I don't anticipate these allocations being a problem. If they are, we can always do the pattern of looking up first, then dispatching to a local function that does the computation if that fails.

new DeferredDocumentationProvider(compilation));
}

private static (AssemblyMetadata, ISupportDirectMemoryAccess?) ComputeMetadata(ITemporaryStreamStorage storage, CancellationToken cancellationToken)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a move from our sibling type.

/// Used to tie lifetime of the underlying direct memory to any metadata reference we pass out. This way the
/// memory will not be GC'ed while this reference is alive.
/// </summary>
private readonly ISupportDirectMemoryAccess? _directMemoryAccess;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reference set type is now very concrete. it holds the real computed data in a strong fashion, and allows it to be found associated both with compilation-trackers (which pass the set forward), or associated with compilations, in case we have compilation-reuse across trackers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this type is now exceedingly trivial as well, as all it does it hold onto this raw data and then create/cache using it to create the actual PEReference objects that are handed out.

@CyrusNajmabadi CyrusNajmabadi requested a review from dibarbet June 2, 2022 19:52
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@dibarbet Can you take a look today? Thanks!

@dibarbet
Copy link
Copy Markdown
Member

dibarbet commented Jun 3, 2022

@dibarbet Can you take a look today? Thanks!

will look this morning!

Copy link
Copy Markdown
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable

@CyrusNajmabadi CyrusNajmabadi merged commit 99195ea into dotnet:main Jun 3, 2022
@ghost ghost added this to the Next milestone Jun 3, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the cacheSkeletonSet branch June 6, 2022 20:09
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants