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 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
@@ -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,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 abstract record Work(ProjectId ProjectId);
internal sealed record UpdateWork(ProjectId ProjectId) : Work(ProjectId);
internal sealed record RemovalWork(ProjectId ProjectId, string IntermediateOutputPath) : Work(ProjectId);
}
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,31 +3,31 @@

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.
private ImmutableDictionary<ProjectId, bool> _projectsWithDynamicFile = ImmutableDictionary<ProjectId, bool>.Empty;

private Stream? _stream;
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)
{
Expand All @@ -42,22 +42,21 @@ void IDisposable.Dispose()
if (_workspace is not null)
{
_workspace.WorkspaceChanged -= Workspace_WorkspaceChanged;
_workspace = null;
}

if (_disposed)
if (_disposeTokenSource.IsCancellationRequested)
{
_logger.LogInformation("Disposal was called twice");
return;
}

_disposed = true;
_logger.LogInformation("Tearing down named pipe for pid {pid}", Process.GetCurrentProcess().Id);

_disposeTokenSource.Cancel();
_disposeTokenSource.Dispose();

_stream?.Dispose();
_stream = null;
}

public void NotifyDynamicFile(ProjectId projectId)
Expand Down Expand Up @@ -93,7 +92,7 @@ private protected void EnsureInitialized(Workspace workspace, Func<Stream> creat
}

// Early check for disposal just to reduce any work further
if (_disposed)
if (_disposeTokenSource.IsCancellationRequested)
{
return;
}
Expand Down Expand Up @@ -173,7 +172,7 @@ private void Workspace_WorkspaceChanged(object? sender, WorkspaceChangeEventArgs
//
void EnqueueUpdate(Project? project)
{
if (_disposed ||
if (_disposeTokenSource.IsCancellationRequested ||
project is not
{
Language: LanguageNames.CSharp
Expand All @@ -195,7 +194,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 All @@ -219,50 +218,47 @@ void RemoveProject(Project project)
/// </remarks>
private protected async virtual ValueTask ProcessWorkAsync(ImmutableArray<Work> work, CancellationToken cancellationToken)
{
// 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?.CurrentSolution;

cancellationToken.ThrowIfCancellationRequested();

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

Choose a reason for hiding this comment

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

Call me paranoid, but I still have flashbacks to the last time a disposed flag was changed to check a cancellation token, and that didn't go well.

If I'm reading this right, the assumption is that stream will never be null because it's only set to null in DIspose, and we're checking cancellation here. BUT unless I missed it, the AsyncBatchingWorkQueue doesn't create a linked cancellation token for an individual batch, and the whole queue. I think it might be possible for us to be disposed, and not necessarily have this token be cancelled.

If I'm wrong, then great, but perhaps a comment explaining why it can't happen would be good. Alternatively, just checking both tokens here might make sense.

Copy link
Member

@DustinCampbell DustinCampbell Sep 6, 2024

Choose a reason for hiding this comment

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

BUT unless I missed it, the AsyncBatchingWorkQueue doesn't create a linked cancellation token for an individual batch, and the whole queue.

The AsyncBatchingWorkQueue does indeed create a linked cancellation token so that a batch will be cancelled when the main cancellation token is cancelled. It uses a CancellationSeries to do exactly that. So, if _disposeTokenSource.Cancel() is called, the cancellation token passed to the batch will also be in a cancelled state.

Here's the relevant code in AsyncBatchingWorkQueue:

public AsyncBatchingWorkQueue(
TimeSpan delay,
Func<ImmutableArray<TItem>, CancellationToken, ValueTask<TResult>> processBatchAsync,
IEqualityComparer<TItem>? equalityComparer,
CancellationToken cancellationToken)
{
_delay = delay;
_processBatchAsync = processBatchAsync;
_equalityComparer = equalityComparer;
_entireQueueCancellationToken = cancellationToken;
_uniqueItems = new HashSet<TItem>(equalityComparer);
// Combine with the queue cancellation token so that any batch is controlled by that token as well.
_cancellationSeries = new CancellationSeries(_entireQueueCancellationToken);
CancelExistingWork();
}
/// <summary>
/// Cancels any outstanding work in this queue. Work that has not yet started will never run. Work that is in
/// progress will request cancellation in a standard best effort fashion.
/// </summary>
public void CancelExistingWork()
{
lock (_gate)
{
// Cancel out the current executing batch, and create a new token for the next batch.
_nextBatchCancellationToken = _cancellationSeries.CreateNext();
// Clear out the existing items that haven't run yet. There is no point keeping them around now.
_nextBatch.Clear();
_uniqueItems.Clear();
}
}

CancellationService.CreateNext() does the work of creating a linked token source.

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

var stream = _stream.AssumeNotNull();
var solution = _workspace.AssumeNotNull().CurrentSolution;

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)
{
try
{
cancellationToken.ThrowIfCancellationRequested();
if (cancellationToken.IsCancellationRequested)
{
return;
}

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 +268,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);
}
}
}
Loading
Loading