Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Reduce NamedPipe Chatter (take 2) #10813
Changes from 6 commits
23ef2a1
9fa70b2
ba2c257
4693b2c
3179c68
54d5a07
1f9d953
14f5d71
734fab6
0dd6a2c
aeebf20
c64d110
2c0bd4d
80b6c89
bf8283c
8fda8d0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 intoProcessWorkAsync
.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.
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
andsolution
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: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?