Skip to content

Add request concurrency information for processing by the RequestExec… #66727

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 13 commits into from
Closed

Add request concurrency information for processing by the RequestExec… #66727

wants to merge 13 commits into from

Conversation

@ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Feb 6, 2023

…utionQueue.

This replaces the MutatesServerState flag with a flag that indicates how the request should be treated. Specifically, I needed a mode where the request ensured that all prior pending requests had completed before processing.

@ToddGrun
Add request concurrency information for processing by the RequestExec…
7c152da 
…utionQueue.

This replaces the MutatesServerState flag with a flag that indicates how the request should be treated. Specifically, I needed a mode where the request ensured that all prior pending requests had completed before processing.
@ghost ghost added the Area-IDE label Feb 6, 2023
@ToddGrun ToddGrun mentioned this pull request Feb 6, 2023
Closed
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Feb 6, 2023 •
edited by ToddGrun
Loading

Specifically, I needed a mode where the request ensured that all prior pending requests had completed before processing.

Can you give an example of where/why you needed this?

In reply to: 1419909945

In reply to: 1419909945

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Feb 6, 2023

Yes, first a bit of context: dotnet/razor#8217

WebTools currently uses the O# WithContentModifiedSupport(false) behavior which ensures that all pending requests are cancelled and that the request is handled serially. WebTools needs this because our parse trees are updated during the didChange notification and our parse trees are immutable. We aren't resilient to operations operating our parse tree at a time when it is being modified. I'll add a comment to the code to better outline this use case.

In reply to: 1419909945

@davidwengier
Copy link
Member

davidwengier commented Feb 6, 2023 •
edited by ToddGrun
Loading

The flags enum doesn't feel right here, to me. Two of the values control how a work item is processed (Parallel and RequiresCompletionBeforeFurtherQueueing), either nonblocking or blocking, and one of them controls the queue state before the work item is processed, either empty or dont care, except there is no explicit option for don't care.

In reply to: 1419935504

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Feb 6, 2023

I agree, it does feel a little odd. Any suggestions for something better?

In reply to: 1419935504

@davidwengier
Copy link
Member

davidwengier commented Feb 7, 2023 •
edited by ToddGrun
Loading

Do you expect to actually need to set RequiresPreviousQueueItemsCancelled on an individual handler, but not on others? Feels like a boolean property of the queue itself.

In reply to: 1419988252

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Feb 7, 2023

Yes, webtools needs this set on the didChange, didOpen, didClose handlers, but not any others.

In reply to: 1419988252

@davidwengier
Copy link
Member

davidwengier commented Feb 7, 2023 •
edited by ToddGrun
Loading

But aren't they the only ones that are mutating? They are in roslyn and razor at least

In reply to: 1419997630

@ToddGrun
Copy link
Contributor Author

ToddGrun commented Feb 7, 2023

I think I understand. You are saying that we could add a virtual bool on RequestExecutionQueue that indicates whether serial behavior requires clearing out the queue, correct?

In reply to: 1419997630

davidwengier
davidwengier approved these changes Feb 7, 2023
View reviewed changes
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@ToddGrun ToddGrun marked this pull request as ready for review February 7, 2023 17:28
@ToddGrun ToddGrun requested a review from a team as a code owner February 7, 2023 17:28
ryanbrandenburg
ryanbrandenburg approved these changes Feb 7, 2023
View reviewed changes
Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Other than the one comment about using the dictionary in the base-case this looks great.

dibarbet
dibarbet reviewed Feb 7, 2023
View reviewed changes
...res/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs
var context = await work.CreateRequestContextAsync(cancellationToken).ConfigureAwait(false);
if (work.MutatesServerState)
{
if (CancelInProgressWorkUponMutatingRequest)
Copy link
Member

@dibarbet dibarbet Feb 7, 2023

Choose a reason for hiding this comment

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

can you add some unit tests that verify that the queue behaves as you expect? e.g. verify previous work completes/cancelled before the mutating item starts, verify queue cancellation still functions correctly with this option, etc.

we have some existing tests here - https://sourceroslyn.io/#Microsoft.CommonLanguageServerProtocol.Framework.UnitTests/RequestExecutionQueueTests.cs,8166c4c6b96c4379
and https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol.UnitTests/Ordering/RequestOrderingTests.cs,20

@ToddGrun
rename some variables.
ad617e8 
Remove an IsCancellationRequested check.
Throw some InvalidOperationExceptions based on unreachable conditions.
jasonmalinowski
jasonmalinowski reviewed Feb 8, 2023
View reviewed changes
...res/LanguageServer/Microsoft.CommonLanguageServerProtocol.Framework/RequestExecutionQueue.cs Outdated

if (!concurrentlyExecutingTasks.TryAdd(currentWorkTask, currentWorkCts))
{
throw new InvalidOperationException($"unable to add {currentWorkTask} into {concurrentlyExecutingTasks}");
Copy link
Member

@jasonmalinowski jasonmalinowski Feb 8, 2023

Choose a reason for hiding this comment

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

Did you mean to use nameof() for those locals? Otherwise I think you're just going to get type names, right?

Copy link
Contributor Author

@ToddGrun ToddGrun Feb 8, 2023

Choose a reason for hiding this comment

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

bah, yes!

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 8, 2023

Choose a reason for hiding this comment

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

also, just do Contract.ThrowIfFalse(...TryAdd(...)); :)

Copy link
Contributor Author

@ToddGrun ToddGrun Feb 8, 2023

Choose a reason for hiding this comment

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

The Contract class isn't available in this project, from what I've seen, thus all the long winded throwing of InvalidOperationExceptions.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 8, 2023

Choose a reason for hiding this comment

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

  1. Ughhhhhhhhhhhhhhhhhhhhhh
  2. Can we not just source-link it in?

Copy link
Contributor Author

@ToddGrun ToddGrun Feb 8, 2023

Choose a reason for hiding this comment

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

At least for the nullability ones, aren't they required to be in a certain namespace for them to work?

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 8, 2023

Choose a reason for hiding this comment

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

what nullability types are you referring to?

Copy link
Contributor Author

@ToddGrun ToddGrun Feb 8, 2023

Choose a reason for hiding this comment

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

NotNullAttribute for example, referenced here

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Feb 8, 2023

Choose a reason for hiding this comment

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

looks like you pull in C:\github\roslyn\src\Compilers\Core\Portable\InternalUtilities\NullableAttributes.cs for that. Which does nothing if you're on netcore (since runtime provides it).

Copy link
Contributor Author

@ToddGrun ToddGrun Feb 8, 2023

Choose a reason for hiding this comment

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

I think I'm confusing System.Runtime.CompilerServices.NullableAttribute with the one linked above.

@ToddGrun
PR feedback, including using Contract, turning on nullable attributes…
f5b1620 
…, using NoThrowAwaitableInternal, and slightly different exception handling
@ryanbrandenburg ryanbrandenburg mentioned this pull request Feb 9, 2023
Merged
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Feb 12, 2023

Abandoning in favor of Ryan's derivation of this, as his has the tests (and the dependency removal that I provided)

@ToddGrun ToddGrun closed this Feb 12, 2023
@mavasani mavasani added the Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. label May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasonmalinowski jasonmalinowski jasonmalinowski left review comments

@davidwengier davidwengier davidwengier approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@dibarbet dibarbet dibarbet left review comments

+1 more reviewer

@ryanbrandenburg ryanbrandenburg ryanbrandenburg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ToddGrun @CyrusNajmabadi @davidwengier @jasonmalinowski @dibarbet @ryanbrandenburg @mavasani