-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize OOP compilation of HasSourceGenerators. #72978
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System.Collections.Frozen; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Linq; | ||
using System.Runtime.CompilerServices; | ||
|
@@ -17,6 +18,11 @@ | |
|
||
namespace Microsoft.CodeAnalysis; | ||
|
||
// Cache of list of analyzer references to whether or not they have source generators. Keyed based off | ||
// IReadOnlyList<AnalyzerReference> so that we can cache the value even as project-states fork based on | ||
// document edits. | ||
using AnalyzerReferenceMap = ConditionalWeakTable<IReadOnlyList<AnalyzerReference>, AsyncLazy<bool>>; | ||
|
||
internal partial class SolutionCompilationState | ||
{ | ||
private sealed record SourceGeneratorMap( | ||
|
@@ -38,7 +44,11 @@ private sealed record SourceGeneratorMap( | |
/// process (if present) and having it make the determination, without the host necessarily loading generators | ||
/// itself. | ||
/// </summary> | ||
private static readonly ConditionalWeakTable<ProjectState, AsyncLazy<bool>> s_hasSourceGeneratorsMap = new(); | ||
private static readonly Dictionary<string, AnalyzerReferenceMap> s_languageToAnalyzerReferenceMap = new() | ||
{ | ||
{ LanguageNames.CSharp, new() }, | ||
{ LanguageNames.VisualBasic, new() }, | ||
}; | ||
|
||
/// <summary> | ||
/// This method should only be called in a .net core host like our out of process server. | ||
|
@@ -94,21 +104,26 @@ static SourceGeneratorMap ComputeSourceGenerators(ProjectState projectState) | |
public async Task<bool> HasSourceGeneratorsAsync(ProjectId projectId, CancellationToken cancellationToken) | ||
{ | ||
var projectState = this.SolutionState.GetRequiredProjectState(projectId); | ||
if (projectState.AnalyzerReferences.Count == 0) | ||
return false; | ||
|
||
if (!s_hasSourceGeneratorsMap.TryGetValue(projectState, out var lazy)) | ||
if (!RemoteSupportedLanguages.IsSupported(projectState.Language)) | ||
return false; | ||
|
||
var analyzerReferenceMap = s_languageToAnalyzerReferenceMap[projectState.Language]; | ||
if (!analyzerReferenceMap.TryGetValue(projectState.AnalyzerReferences, out var lazy)) | ||
{ | ||
// Extracted into local function to prevent allocations in the case where we find a value in the cache. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my guess is no. there are needed captures in these paths. but i will try. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, it's all sorts of captury. :) No go on static here. We need to capture enough data to be able to call to oop effectively to answer the question. note that once this happens once, and is cached, that captured data will be dropped. So we only need this alloc when looking at a fresh (non-identity) list of analyzer-references. Which should be very rare as the solution forks forwards. |
||
lazy = GetLazy(projectState); | ||
lazy = GetLazy(analyzerReferenceMap, projectState); | ||
} | ||
|
||
return await lazy.GetValueAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
AsyncLazy<bool> GetLazy(ProjectState projectState) | ||
=> s_hasSourceGeneratorsMap.GetValue( | ||
projectState, | ||
projectState => AsyncLazy.Create( | ||
static (tuple, cancellationToken) => ComputeHasSourceGeneratorsAsync(tuple.@this, tuple.projectState, cancellationToken), | ||
(@this: this, projectState))); | ||
AsyncLazy<bool> GetLazy(AnalyzerReferenceMap analyzerReferenceMap, ProjectState projectState) | ||
=> analyzerReferenceMap.GetValue( | ||
projectState.AnalyzerReferences, | ||
_ => AsyncLazy.Create( | ||
cancellationToken => ComputeHasSourceGeneratorsAsync(this, projectState, cancellationToken))); | ||
|
||
static async Task<bool> ComputeHasSourceGeneratorsAsync( | ||
SolutionCompilationState solution, ProjectState projectState, CancellationToken cancellationToken) | ||
|
@@ -120,10 +135,13 @@ static async Task<bool> ComputeHasSourceGeneratorsAsync( | |
|
||
// Out of process, call to the remote to figure this out. | ||
var projectId = projectState.Id; | ||
var projectStateChecksums = await projectState.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); | ||
var analyzerReferences = projectStateChecksums.AnalyzerReferences.Children; | ||
|
||
var result = await client.TryInvokeAsync<IRemoteSourceGenerationService, bool>( | ||
solution, | ||
projectId, | ||
(service, solution, cancellationToken) => service.HasGeneratorsAsync(solution, projectId, cancellationToken), | ||
(service, solution, cancellationToken) => service.HasGeneratorsAsync(solution, projectId, analyzerReferences, projectState.Language, cancellationToken), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new approach passes along the analyzer-reference checksums and language, as we can answer on the oop side with just that info. |
||
cancellationToken).ConfigureAwait(false); | ||
return result.HasValue && result.Value; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,16 +3,23 @@ | |
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Linq; | ||
using System.Runtime.CompilerServices; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
using Microsoft.CodeAnalysis.PooledObjects; | ||
using Microsoft.CodeAnalysis.Shared.Extensions; | ||
using Microsoft.CodeAnalysis.SourceGeneration; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.Remote; | ||
|
||
// Can use AnalyzerReference as a key here as we will will always get back the same instance back for the same checksum. | ||
using AnalyzerReferenceMap = ConditionalWeakTable<AnalyzerReference, StrongBox<bool>>; | ||
|
||
internal sealed partial class RemoteSourceGenerationService(in BrokeredServiceBase.ServiceConstructionArguments arguments) | ||
: BrokeredServiceBase(arguments), IRemoteSourceGenerationService | ||
{ | ||
|
@@ -64,11 +71,65 @@ public ValueTask<ImmutableArray<string>> GetContentsAsync( | |
}, cancellationToken); | ||
} | ||
|
||
public ValueTask<bool> HasGeneratorsAsync(Checksum solutionChecksum, ProjectId projectId, CancellationToken cancellationToken) | ||
private static readonly Dictionary<string, (AnalyzerReferenceMap analyzerReferenceMap, AnalyzerReferenceMap.CreateValueCallback callback)> s_languageToAnalyzerReferenceMap = new() | ||
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
return RunServiceAsync(solutionChecksum, async solution => | ||
{ LanguageNames.CSharp, (new(), static analyzerReference => HasSourceGenerators(analyzerReference, LanguageNames.CSharp)) }, | ||
{ LanguageNames.VisualBasic, (new(), static analyzerReference => HasSourceGenerators(analyzerReference, LanguageNames.VisualBasic)) }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here. we can hardcode in C#/vb here to simplify thigns. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: we cache thsi per analyzer reference so that multiple projects with the same analyzer references (common), and lang, will cache the same result. |
||
}; | ||
|
||
private static StrongBox<bool> HasSourceGenerators( | ||
AnalyzerReference analyzerReference, string language) | ||
{ | ||
var generators = analyzerReference.GetGenerators(language); | ||
return new(generators.Any()); | ||
} | ||
|
||
public async ValueTask<bool> HasGeneratorsAsync( | ||
Checksum solutionChecksum, | ||
ProjectId projectId, | ||
ImmutableArray<Checksum> analyzerReferenceChecksums, | ||
string language, | ||
CancellationToken cancellationToken) | ||
{ | ||
if (analyzerReferenceChecksums.Length == 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically not necessary. the host side already checked this. d |
||
return false; | ||
|
||
// Do not use RunServiceAsync here. We don't want to actually synchronize a solution instance on this remote | ||
// side to service this request. Specifically, solution syncing is expensive, and will pull over a lot of data | ||
// that we don't need (like document contents). All we need to do is synchronize over the analyzer-references | ||
// (which are actually quite small as they are represented as file-paths), and then answer the question based on | ||
// them directly. We can then cache that result for future requests. | ||
var workspace = GetWorkspace(); | ||
var assetProvider = workspace.CreateAssetProvider(solutionChecksum, WorkspaceManager.SolutionAssetCache, SolutionAssetSource); | ||
|
||
using var _1 = PooledHashSet<Checksum>.GetInstance(out var checksums); | ||
checksums.AddRange(analyzerReferenceChecksums); | ||
|
||
// Fetch the analyzer references specified by the host. Note: this will only serialize this information over | ||
// the first time needed. After that, it will be cached in the WorkspaceManager.SolutionAssetCache on the remote | ||
// side, so it will be a no-op to fetch them in the future. | ||
// | ||
// From this point on, the host won't call into us for the same project-state (as it caches the data itself). If | ||
// the project state changes, it will just call into us with the checksums for its analyzer references. As | ||
// those will almost always be the same, we'll just fetch the precomputed values on our end, return them, and | ||
// the host will cache it. We'll only actually fetch something new and compute something new when an actual new | ||
// analyzer reference is added. | ||
using var _2 = ArrayBuilder<AnalyzerReference>.GetInstance(checksums.Count, out var analyzerReferences); | ||
await assetProvider.GetAssetsAsync<AnalyzerReference, ArrayBuilder<AnalyzerReference>>( | ||
projectId, | ||
checksums, | ||
static (_, analyzerReference, analyzerReferences) => analyzerReferences.Add(analyzerReference), | ||
analyzerReferences, | ||
cancellationToken).ConfigureAwait(false); | ||
|
||
var (analyzerReferenceMap, callback) = s_languageToAnalyzerReferenceMap[language]; | ||
foreach (var analyzerReference in analyzerReferences) | ||
{ | ||
return await solution.CompilationState.HasSourceGeneratorsAsync(projectId, cancellationToken).ConfigureAwait(false); | ||
}, cancellationToken); | ||
var hasGenerators = analyzerReferenceMap.GetValue(analyzerReference, callback); | ||
if (hasGenerators.Value) | ||
return true; | ||
} | ||
|
||
return false; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine to have these be keyed just by VB/C#. OOP is only supported for those two languages alone. This makes it easy to initialize and look into this without locking, or complex types like ConcurrentDict.