-
Notifications
You must be signed in to change notification settings - Fork 199
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
base: main
Are you sure you want to change the base?
Conversation
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.
I took a quick look but have to head out for the day. I'll provide more feedback tomorrow.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs
Outdated
Show resolved
Hide resolved
...zor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/DocumentSnapshotHandle.cs
Outdated
Show resolved
Hide resolved
...zor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/DocumentSnapshotHandle.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Outdated
Show resolved
Hide resolved
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.
The benefit is to reduce work done when unrelated changes are made (such as adding/changing a csharp file)
Nit: such as adding/changing an unrelated csharp file. Some C# files we absolutely need to react to (and this PR looks good and will react as appropriate)
I didn't look at any actual serialization changes as partly I assume there are none, and partly I have no idea what to look for.
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorConfiguration.cs
Outdated
Show resolved
Hide resolved
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Outdated
Show resolved
Hide resolved
...zor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Serialization/DocumentSnapshotHandle.cs
Outdated
Show resolved
Hide resolved
…orProjectInfoHelpers.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/RazorProjectInfoHelpers.cs
Show resolved
Hide resolved
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.
LGTM, based on your changes it seems like using checksums helps optimize the synchronization process.
Question, how frequently the checksum recalculated? is it done on every change or if else what is the main trigger?
Every time we get a workspace update we have to fully calculate the checksum. The reduction here would be not in computation on the workspace listener, but in work done for updating project information and serialization/deserialization. |
.../src/Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/CachedTagHelperResolver.cs
Outdated
Show resolved
Hide resolved
…Workspace/RazorWorkspaceListenerBase.cs Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>
{ | ||
if (checksum == projectInfo.Checksum) | ||
{ | ||
_logger.LogInformation("Checksum for {projectId} did not change. Skipped sending update", project.Id); |
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.
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?
@@ -227,18 +228,18 @@ private protected async virtual ValueTask ProcessWorkAsync(ImmutableArray<Work> | |||
|
|||
cancellationToken.ThrowIfCancellationRequested(); |
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.
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
.
@@ -227,18 +228,18 @@ private protected async virtual ValueTask ProcessWorkAsync(ImmutableArray<Work> | |||
|
|||
cancellationToken.ThrowIfCancellationRequested(); | |||
|
|||
// 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) |
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.
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.
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.
I removed disposed here and hopefully made this more clear.
...Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.Work.cs
Outdated
Show resolved
Hide resolved
...Microsoft.AspNetCore.Razor.ExternalAccess.RoslynWorkspace/RazorWorkspaceListenerBase.Work.cs
Outdated
Show resolved
Hide resolved
@@ -227,18 +228,18 @@ private protected async virtual ValueTask ProcessWorkAsync(ImmutableArray<Work> | |||
|
|||
cancellationToken.ThrowIfCancellationRequested(); | |||
|
|||
// 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) |
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.
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.
_disposed
is always set to true before_stream
is set to null. So,stream
could never be null._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);
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.
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.
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.
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
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.
Absolutely! I just wanted to note some thoughts while reviewing. That feedback can definitely wait until later!
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.
This definitely improves things. I some additional feedback, but most of it is unrelated to the goal of this PR. So, you can feel free to take it or leave it. I did note that there's some code in WorkspaceProjectStateChangeDetector.cs
that might be helpful in reducing updates here as well.
|
||
// 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) |
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.
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.
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.
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
:
Lines 105 to 138 in 07e1382
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.
Moving to draft. This has some good ideas in it but want to re-evaluate this in light of recent conversations about architecture |
2nd attempt, first was #10756
This uses checksums for RazorProjectInformation to determine if it should be serialized and sent to razor. The benefit is to reduce work done when unrelated changes are made (such as adding/changing a csharp file)