Skip to content
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

Merged
merged 5 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ ValueTask<ImmutableArray<string>> GetContentsAsync(
Checksum solutionChecksum, ProjectId projectId, ImmutableArray<DocumentId> documentIds, CancellationToken cancellationToken);

/// <summary>
/// Whether or not the specified <paramref name="projectId"/> has source generators or not.
/// Whether or not the specified analyzer references have source generators or not.
/// </summary>
ValueTask<bool> HasGeneratorsAsync(
Checksum solutionChecksum, ProjectId projectId, CancellationToken cancellationToken);
Checksum solutionChecksum, ProjectId projectId, ImmutableArray<Checksum> analyzerReferenceChecksums, string language, CancellationToken cancellationToken);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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(
Expand All @@ -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() },
Copy link
Member Author

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.

};

/// <summary>
/// This method should only be called in a .net core host like our out of process server.
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

// Extracted into local function to prevent allocations in the case where we find a value in the cache.

Could we just make static and get rid of this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand All @@ -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),
Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}
Expand Down
4 changes: 4 additions & 0 deletions src/Workspaces/Remote/ServiceHub/Host/AssetProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ public override async ValueTask GetAssetsAsync<T, TArg>(
await this.SynchronizeAssetsAsync(assetPath, checksums, callback, arg, cancellationToken).ConfigureAwait(false);
}

/// <summary>
/// This is the function called when we are <em>not</em> doing an incremental update, but are instead doing a bulk
/// full sync.
/// </summary>
public async ValueTask SynchronizeSolutionAssetsAsync(Checksum solutionChecksum, CancellationToken cancellationToken)
{
var timer = SharedStopwatch.StartNew();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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()
{
return RunServiceAsync(solutionChecksum, async solution =>
{ LanguageNames.CSharp, (new(), static analyzerReference => HasSourceGenerators(analyzerReference, LanguageNames.CSharp)) },
{ LanguageNames.VisualBasic, (new(), static analyzerReference => HasSourceGenerators(analyzerReference, LanguageNames.VisualBasic)) },
Copy link
Member Author

Choose a reason for hiding this comment

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

same here. we can hardcode in C#/vb here to simplify thigns.

Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Copy link
Member Author

Choose a reason for hiding this comment

The 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;
}
}
Loading