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

Reduce NamedPipe Chatter (take 2) #10813

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 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
@@ -1,8 +1,12 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.Extensions.Internal;

namespace Microsoft.AspNetCore.Razor.Language;
Expand All @@ -21,6 +25,10 @@ public sealed record class RazorConfiguration(
LanguageServerFlags: null,
UseConsolidatedMvcViews: true);

private Checksum? _checksum;
internal Checksum Checksum
=> _checksum ?? InterlockedOperations.Initialize(ref _checksum, CalculateChecksum());
DustinCampbell marked this conversation as resolved.
Show resolved Hide resolved

public bool Equals(RazorConfiguration? other)
DustinCampbell marked this conversation as resolved.
Show resolved Hide resolved
=> other is not null &&
LanguageVersion == other.LanguageVersion &&
Expand All @@ -39,4 +47,33 @@ public override int GetHashCode()
hash.Add(LanguageServerFlags);
return hash;
}

internal void AppendChecksum(Checksum.Builder builder)
{
builder.AppendData(LanguageVersion.Major);
builder.AppendData(LanguageVersion.Minor);
builder.AppendData(ConfigurationName);
builder.AppendData(UseConsolidatedMvcViews);

if (LanguageServerFlags is null)
{
builder.AppendNull();
}
else
{
builder.AppendData(LanguageServerFlags.ForceRuntimeCodeGeneration);
}

foreach (var extension in Extensions)
{
builder.AppendData(extension.ExtensionName);
}
}

private Checksum CalculateChecksum()
{
var builder = new Checksum.Builder();
AppendChecksum(builder);
return builder.FreeAndGetChecksum();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using Microsoft.AspNetCore.Razor.Utilities;

namespace Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace;

public abstract partial class RazorWorkspaceListenerBase
{
internal class ProjectEntry
{
public int? TagHelpersResultId { get; set; }
public Checksum? ProjectChecksum { get; set; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using Microsoft.CodeAnalysis;

namespace Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace;

public abstract partial class RazorWorkspaceListenerBase
{
internal record Work(ProjectId ProjectId);
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
internal record UpdateWork(ProjectId ProjectId) : Work(ProjectId);
internal record RemovalWork(ProjectId ProjectId, string IntermediateOutputPath) : Work(ProjectId);
ryzngard marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider taking a look at WorkspaceProjectStateChangeDetector in the VS layer. This class also does some extra work to avoid enqueuing work. For example, for document changes, it does work to determine whether the change will affect components and tag helpers. Those extra checks might be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me do that in a follow up. Definitely think there can be some shared code there but it might be better to move it around and unify

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely! I just wanted to note some thoughts while reviewing. That feedback can definitely wait until later!

Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,24 @@

using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.AspNetCore.Razor.Telemetry;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace;

public abstract class RazorWorkspaceListenerBase : IDisposable
public abstract partial class RazorWorkspaceListenerBase : IDisposable
{
private static readonly TimeSpan s_debounceTime = TimeSpan.FromMilliseconds(500);
private readonly CancellationTokenSource _disposeTokenSource = new();

private readonly ILogger _logger;
private readonly AsyncBatchingWorkQueue<Work> _workQueue;
private readonly CompilationTagHelperResolver _tagHelperResolver = new(NoOpTelemetryReporter.Instance);

// Only modified in the batching work queue so no need to lock for mutation
private readonly Dictionary<ProjectId, Checksum?> _projectChecksums = new();

// Use an immutable dictionary for ImmutableInterlocked operations. The value isn't checked, just
// the existance of the key so work is only done for projects with dynamic files.
Expand All @@ -25,10 +30,6 @@ public abstract class RazorWorkspaceListenerBase : IDisposable
private Workspace? _workspace;
private bool _disposed;

internal record Work(ProjectId ProjectId);
internal record UpdateWork(ProjectId ProjectId) : Work(ProjectId);
internal record RemovalWork(ProjectId ProjectId, string IntermediateOutputPath) : Work(ProjectId);

private protected RazorWorkspaceListenerBase(ILogger logger)
{
_logger = logger;
Expand Down Expand Up @@ -195,7 +196,7 @@ void RemoveProject(Project project)
{
// Remove project is called from Workspace.Changed, while other notifications of _projectsWithDynamicFile
// are handled with NotifyDynamicFile. Use ImmutableInterlocked here to be sure the updates happen
// in a thread safe manner since those are not assumed to be the same thread.
// in a thread safe manner since those are not assumed to be the same thread.
if (ImmutableInterlocked.TryRemove(ref _projectsWithDynamicFile, project.Id, out var _))
{
var intermediateOutputPath = Path.GetDirectoryName(project.CompilationOutputInfo.AssemblyPath);
Expand Down Expand Up @@ -227,18 +228,18 @@ private protected async virtual ValueTask ProcessWorkAsync(ImmutableArray<Work>

cancellationToken.ThrowIfCancellationRequested();
Copy link
Member

@DustinCampbell DustinCampbell Sep 5, 2024

Choose a reason for hiding this comment

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

Consider just checking if cancellation is requested and returning. Throwing on cancellation shouldn't be necessary from the "process batch" function of an async work queue.

Same feedback for ProcessWorkCoreAsync below, which feels to me like it could probably be inlined into ProcessWorkAsync.


// Early bail check for if we are disposed or somewhere in the middle of disposal
// Early bail check for if we are disposed or somewhere in the middle of disposal
if (_disposed || stream is null || solution is null)
Copy link
Member

@DustinCampbell DustinCampbell Sep 5, 2024

Choose a reason for hiding this comment

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

Unrelated to this PR, but I just noticed that this class has both _disposed and _disposeTokenSource fields. Is the extra _disposed field actually needed? Isn't _disposeTokenSource.IsCanellationRequested sufficient to check for disposal rather than having two bits of data? I would also imagine that _disposeTokenSource is better from a thread-safety perspective, though that really doesn't matter for this class, since _disposed is only set in a single location and there's no risk of tearing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed disposed here and hopefully made this more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Another thought: Why is checking stream and solution for null here? They were assigned to locals at the start of the method, and I don't think they can ever be null at this point.

  1. _disposed is always set to true before _stream is set to null. So, stream could never be null.
  2. _workspace is never set to null, and no work is enqueued until _workspace is provably non-null. So, _workspace?.CurrentSolution can never be null when processing a batch of work.

To clarify the invariants, considering use AssumeNotNull() before and after the _disposed check to clear up avoid nullability warnings:

// Capture as locals here. Cancellation of the work queue still need to propogate. The cancellation
// token itself represents the work queue halting, but this will help avoid any assumptions about nullability of locals
// through the use in this function.
var stream = _stream;
var solution = _workspace.AssumeNotNull().CurrentSolution;

cancellationToken.ThrowIfCancellationRequested();

// Early bail check for if we are disposed or somewhere in the middle of disposal
if (_disposed)
{
    _logger.LogTrace("Skipping work due to disposal");
    return;
}

stream.AssumeNotNull();

await CheckConnectionAsync(stream, cancellationToken).ConfigureAwait(false);

{
_logger.LogTrace("Skipping work due to disposal");
return;
}

await CheckConnectionAsync(stream, cancellationToken).ConfigureAwait(false);
await ProcessWorkCoreAsync(work, stream, solution, _logger, cancellationToken).ConfigureAwait(false);
await ProcessWorkCoreAsync(work, stream, solution, cancellationToken).ConfigureAwait(false);
}

private static async Task ProcessWorkCoreAsync(ImmutableArray<Work> work, Stream stream, Solution solution, ILogger logger, CancellationToken cancellationToken)
private async Task ProcessWorkCoreAsync(ImmutableArray<Work> work, Stream stream, Solution solution, CancellationToken cancellationToken)
{
foreach (var unit in work)
{
Expand All @@ -248,21 +249,21 @@ private static async Task ProcessWorkCoreAsync(ImmutableArray<Work> work, Stream

if (unit is RemovalWork removalWork)
{
await ReportRemovalAsync(stream, removalWork, logger, cancellationToken).ConfigureAwait(false);
await ReportRemovalAsync(stream, removalWork, _logger, cancellationToken).ConfigureAwait(false);
}

var project = solution.GetProject(unit.ProjectId);
if (project is null)
{
logger.LogTrace("Project {projectId} is not in workspace", unit.ProjectId);
_logger.LogTrace("Project {projectId} is not in workspace", unit.ProjectId);
continue;
}

await ReportUpdateProjectAsync(stream, project, logger, cancellationToken).ConfigureAwait(false);
await ReportUpdateProjectAsync(stream, project, cancellationToken).ConfigureAwait(false);
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
logger.LogError(ex, "Encountered exception while processing unit: {message}", ex.Message);
_logger.LogError(ex, "Encountered exception while processing unit: {message}", ex.Message);
}
}

Expand All @@ -272,22 +273,37 @@ private static async Task ProcessWorkCoreAsync(ImmutableArray<Work> work, Stream
}
catch (Exception ex)
{
logger.LogError(ex, "Encountered error flusingh stream");
_logger.LogError(ex, "Encountered error flushing stream");
}
}

private static async Task ReportUpdateProjectAsync(Stream stream, Project project, ILogger logger, CancellationToken cancellationToken)
private async Task ReportUpdateProjectAsync(Stream stream, Project project, CancellationToken cancellationToken)
{
logger.LogTrace("Serializing information for {projectId}", project.Id);
var projectInfo = await RazorProjectInfoFactory.ConvertAsync(project, logger, cancellationToken).ConfigureAwait(false);
if (projectInfo is null)
_logger.LogTrace("Serializing information for {projectId}", project.Id);
var projectPath = Path.GetDirectoryName(project.FilePath);
if (projectPath is null)
{
logger.LogTrace("Skipped writing data for {projectId}", project.Id);
_logger.LogInformation("projectPath is null, skip update for {projectId}", project.Id);
return;
}

stream.WriteProjectInfoAction(ProjectInfoAction.Update);
await stream.WriteProjectInfoAsync(projectInfo, cancellationToken).ConfigureAwait(false);
var checksum = _projectChecksums.GetOrAdd(project.Id, static _ => null);
var projectEngine = RazorProjectInfoHelpers.GetProjectEngine(project, projectPath);
var tagHelpers = await _tagHelperResolver.GetTagHelpersAsync(project, projectEngine, cancellationToken).ConfigureAwait(false);
var projectInfo = RazorProjectInfoHelpers.TryConvert(project, projectPath, tagHelpers);
if (projectInfo is not null)
{
if (checksum == projectInfo.Checksum)
{
_logger.LogInformation("Checksum for {projectId} did not change. Skipped sending update", project.Id);
Copy link
Member

Choose a reason for hiding this comment

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

In practice, won't this be quite noisy? In a project containing razor files, wouldn't it be sent for any text change in a C# file that doesn't affect tag helpers?

return;
}

_projectChecksums[project.Id] = projectInfo.Checksum;

stream.WriteProjectInfoAction(ProjectInfoAction.Update);
await stream.WriteProjectInfoAsync(projectInfo, cancellationToken).ConfigureAwait(false);
}
}

private static Task ReportRemovalAsync(Stream stream, RemovalWork unit, ILogger logger, CancellationToken cancellationToken)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

#if !NET
using System;
#endif

using System.Diagnostics;
using Microsoft.AspNetCore.Razor.ProjectSystem;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis;

namespace Microsoft.AspNetCore.Razor;

internal static class ProjectExtensions
{
public static ProjectKey ToProjectKey(this Project project)
{
var intermediateOutputPath = FilePathNormalizer.GetNormalizedDirectoryName(project.CompilationOutputInfo.AssemblyPath);
return new(intermediateOutputPath);
}

/// <summary>
/// Returns <see langword="true"/> if this <see cref="ProjectKey"/> matches the given <see cref="Project"/>.
/// </summary>
public static bool Matches(this ProjectKey projectKey, Project project)
{
// In order to perform this check, we are relying on the fact that Id will always end with a '/',
// because it is guaranteed to be normalized. However, CompilationOutputInfo.AssemblyPath will
// contain the assembly file name, which AreDirectoryPathsEquivalent will shave off before comparing.
// So, AreDirectoryPathsEquivalent will return true when Id is "C:/my/project/path/"
// and the assembly path is "C:\my\project\path\assembly.dll"

Debug.Assert(projectKey.Id.EndsWith('/'), $"This method can't be called if {nameof(projectKey.Id)} is not a normalized directory path.");

return FilePathNormalizer.AreDirectoryPathsEquivalent(projectKey.Id, project.CompilationOutputInfo.AssemblyPath);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Immutable;
using System.Linq;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.Extensions.Internal;

Expand Down Expand Up @@ -54,4 +55,13 @@ public override int GetHashCode()

return hash.CombinedHash;
}

internal void AppendChecksum(Checksum.Builder builder)
{
builder.AppendData((int)CSharpLanguageVersion);
foreach (var tagHelper in TagHelpers)
{
builder.AppendData(tagHelper.Checksum);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,30 +1,16 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT license. See License.txt in the project root for license information.

using System;
using System.Buffers;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using MessagePack;
using MessagePack.Resolvers;
using Microsoft.AspNetCore.Razor.Language;
using Microsoft.AspNetCore.Razor.Serialization;
using Microsoft.AspNetCore.Razor.Serialization.MessagePack.Resolvers;
using Microsoft.AspNetCore.Razor.Utilities;
using Microsoft.CodeAnalysis;
using Microsoft.Extensions.Internal;

namespace Microsoft.AspNetCore.Razor.ProjectSystem;

internal sealed record class RazorProjectInfo
{
private static readonly MessagePackSerializerOptions s_options = MessagePackSerializerOptions.Standard
.WithResolver(CompositeResolver.Create(
RazorProjectInfoResolver.Instance,
StandardResolver.Instance));

public ProjectKey ProjectKey { get; init; }
public string FilePath { get; init; }
public RazorConfiguration Configuration { get; init; }
Expand All @@ -33,6 +19,10 @@ internal sealed record class RazorProjectInfo
public ProjectWorkspaceState ProjectWorkspaceState { get; init; }
public ImmutableArray<DocumentSnapshotHandle> Documents { get; init; }

private Checksum? _checksum;
internal Checksum Checksum
=> _checksum ?? InterlockedOperations.Initialize(ref _checksum, ComputeChecksum());

public RazorProjectInfo(
ProjectKey projectKey,
string filePath,
Expand All @@ -51,46 +41,29 @@ public RazorProjectInfo(
Documents = documents.NullToEmpty();
}

public bool Equals(RazorProjectInfo? other)
=> other is not null &&
ProjectKey == other.ProjectKey &&
FilePath == other.FilePath &&
Configuration.Equals(other.Configuration) &&
RootNamespace == other.RootNamespace &&
DisplayName == other.DisplayName &&
ProjectWorkspaceState.Equals(other.ProjectWorkspaceState) &&
Documents.SequenceEqual(other.Documents);
public bool Equals(RazorConfiguration? other)
=> other is not null && Checksum.Equals(other.Checksum);

public override int GetHashCode()
{
var hash = HashCodeCombiner.Start();
=> Checksum.GetHashCode();

hash.Add(ProjectKey);
hash.Add(FilePath);
hash.Add(Configuration);
hash.Add(RootNamespace);
hash.Add(DisplayName);
hash.Add(ProjectWorkspaceState);
hash.Add(Documents);

return hash.CombinedHash;
}

public byte[] Serialize()
=> MessagePackSerializer.Serialize(this, s_options);

public void SerializeTo(IBufferWriter<byte> bufferWriter)
=> MessagePackSerializer.Serialize(bufferWriter, this, s_options);
private Checksum ComputeChecksum()
{
var builder = new Checksum.Builder();

public void SerializeTo(Stream stream)
=> MessagePackSerializer.Serialize(stream, this, s_options);
builder.AppendData(FilePath);
builder.AppendData(ProjectKey.Id);
builder.AppendData(DisplayName);
builder.AppendData(RootNamespace);

public static RazorProjectInfo? DeserializeFrom(ReadOnlyMemory<byte> buffer)
=> MessagePackSerializer.Deserialize<RazorProjectInfo>(buffer, s_options);
Configuration.AppendChecksum(builder);
foreach (var document in Documents)
{
document.AppendChecksum(builder);
}

public static RazorProjectInfo? DeserializeFrom(Stream stream)
=> MessagePackSerializer.Deserialize<RazorProjectInfo>(stream, s_options);
ProjectWorkspaceState.AppendChecksum(builder);

public static ValueTask<RazorProjectInfo> DeserializeFromAsync(Stream stream, CancellationToken cancellationToken)
=> MessagePackSerializer.DeserializeAsync<RazorProjectInfo>(stream, s_options, cancellationToken);
return builder.FreeAndGetChecksum();
}
}
Loading
Loading