Replace `BatchingWorkQueue` with `AsyncBatchingWorkQueue` from Roslyn #10140

DustinCampbell · 2024-03-21T19:25:22Z

I recommend reviewing commit-by-commit.

Razor has had a BatchingWorkQueue for a long while that has dubious semantics. It is used by three features:

OpenDocumentGenerator - Listens for document updates, computes generated output for each, and notifies listeners when a document has been processed.
WorkspaceSemanticTokensRefreshPublisher - Called in the server to send the client semanticTokens/refresh notifications. This really sort of abuses BatchingWorkQueue just for debouncing and only send notifications every 250 milliseconds.
WorkspaceProjectStateChangeDetector - Listens for Roslyn workspace and Razor project manager changes and calls into IProjectWorkspaceStateGenerator.Update(...) when a change occurs.

This change fully replaces BatchingWorkQueue with a copy of Roslyn's AsyncBatchingWorkQueue, which has consistent semantics and has been battle-hardened for years. I've updated OpenDocumentGenerator and WorkspaceProjectStateChangeDetector above to use AsyncBatchingWorkQueue. For WorkspaceSemanticTokensRefreshPublisher (now WorkspaceSemanticTokensRefreshNotifier), I've removed the BatchingWorkQueue altogether and replaced it with simpler logic that debounces and calls Task.Delay(...).

Many thanks to @CyrusNajmabadi for his help with AsyncBatchingWorkQueue.

@dotnet/razor-compiler: The only relevant compiler changes are in shared code.

DustinCampbell · 2024-03-21T20:58:38Z

/azp run

azure-pipelines · 2024-03-21T20:58:53Z

Azure Pipelines successfully started running 2 pipeline(s).

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

...zor/src/Microsoft.VisualStudio.LanguageServices.Razor/WorkspaceProjectStateChangeDetector.cs

src/Razor/src/Microsoft.AspNetCore.Razor.ProjectEngineHost/Threading/CancellationSeries.cs

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

Approval was unintentional added to draft PR

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs

Roslyn's `AsyncBatchingWorkQueue` is a battle-hardened utility that provides a robust mechanism to do work in batches without blocking.

…ations `WorkspaceSemanticTokensRefreshNotifier` (previously `WorkspaceSemanticTokensRefreshPublisher`) uses a `BatchingWorkQueue` for debouncing, even though it isn't actually batching work. I've replaced this with much simpler code that just tracks a `Task` generated with `Task.Delay(...)`.

…ueue

Remove invalid link from comment and make code style consistent.

…yncBatchingWorkQueue

DustinCampbell · 2024-03-25T21:12:41Z

@CyrusNajmabadi, @davidwengier, @jjonescz: I've made further changes to pull the "most recent item" de-duping out of AsyncBatchingWorkQueue and into a helper method, if you're interested in taking another look.

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared.Test/ImmutableArrayExtensionsTests.cs

davidwengier · 2024-03-25T21:21:21Z

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/ImmutableArrayExtensions.cs

+#if !NETSTANDARD2_0
+        var uniqueItems = new HashSet<T>(capacity: source.Length, comparer);
+#else
+        var uniqueItems = new HashSet<T>(comparer);
+#endif


Can/Should this be pooled?

Unfortunately, no. It can't because of the comparer. Like Dictionary<TKey, TValue>, there's no way to set a HashSet<T> comparer other than in the constructor.

davidwengier · 2024-03-25T21:21:57Z

Still LGTM

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared.Test/ImmutableArrayExtensionsTests.cs

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/ImmutableArrayExtensions.cs

RikkiGibson · 2024-03-27T21:56:16Z

Heads up that there might be an RPS regression from this

Known good: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/538963
Known bad: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/539115
See details in the known bad.

DustinCampbell · 2024-03-27T23:03:04Z

Heads up that there might be an RPS regression from this

Known good: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/538963 Known bad: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/539115 See details in the known bad.

Possibly, but I'm not sure. There were similar RPS regressions in our 17.10p3 payload that have been fixed in the release/dev17.10 branch. We should get those fixes into main to be sure.

davidwengier · 2024-03-28T01:30:07Z

The fixes have merged to main

DustinCampbell · 2024-03-28T15:02:58Z

@RikkiGibson: Now that main has the release/dev17.10 fixes, is there an updated PR we can track?

RikkiGibson · 2024-03-28T17:56:14Z

Latest main insertion is showing fewer assembly loads but still elevated from baseline https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/539887

DustinCampbell · 2024-03-28T18:16:11Z

I think that's going to require further investigation. Could you file an issue, start a mail thread, or raise this in Teams? It feels like the right eyes aren't going to see this on a closed PR. I'll start digging in though.

RikkiGibson · 2024-03-28T18:32:24Z

Started the conversation on the infraswat side. Thanks!

DustinCampbell · 2024-03-28T18:44:20Z

OK. I'm not on infraswat and probably won't see that. However, I suspect that @phil-allen-msft is. Just know that I'm actively investigating and working on a fix.

DustinCampbell requested review from a team as code owners March 21, 2024 19:25

DustinCampbell marked this pull request as draft March 21, 2024 19:46

DustinCampbell force-pushed the work-queues branch from 0bdcd7e to 1ef11e6 Compare March 21, 2024 20:18

DustinCampbell changed the title ~~Replace BatchingWorkQueue with AsyncBatchingWorkQueue from Roslyn~~ [DRAFT] Replace BatchingWorkQueue with AsyncBatchingWorkQueue from Roslyn Mar 21, 2024

DustinCampbell force-pushed the work-queues branch 3 times, most recently from ea8db69 to 21dbfae Compare March 21, 2024 20:47

DustinCampbell force-pushed the work-queues branch 4 times, most recently from c1f8f49 to 2814950 Compare March 21, 2024 23:49

davidwengier previously approved these changes Mar 22, 2024

View reviewed changes

DustinCampbell force-pushed the work-queues branch from 2814950 to d49ff1e Compare March 22, 2024 00:16

DustinCampbell requested a review from davidwengier March 22, 2024 00:25

DustinCampbell requested a review from a team March 22, 2024 01:31

davidwengier reviewed Mar 22, 2024

View reviewed changes

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/WorkspaceSemanticTokensRefreshNotifier.cs Outdated Show resolved Hide resolved

DustinCampbell added 10 commits March 22, 2024 09:33

Copy AsyncBatchingWorkQueue from Roslyn

530fd6d

Roslyn's `AsyncBatchingWorkQueue` is a battle-hardened utility that provides a robust mechanism to do work in batches without blocking.

Rewrite OpenDocumentGenerator to use AsyncBatchingWorkQueue

52fe75d

Rewrite WorkspaceProjectStateChangeDetector to use AsyncBatchingWorkQ…

7b4733d

…ueue

Add a few AsyncBatchingWorkQueue tests

f9f7d17

Delete old BatchingWorkQueue

536fc08

Small tweaks to CancellationSeries

e710932

Remove invalid link from comment and make code style consistent.

Cache SemanticTokens.RefreshSupport value and check it sooner

286bfc7

Pass cancellation token to Task.Delay

2a3c3c2

Provide AsyncBatchingWorkQueue mechansim to control deduping

b5de994

DustinCampbell added 3 commits March 25, 2024 13:43

Add ImmutableArray<T>.GetMostRecentUniqueItems() helper

7456e25

Use GetMostRecentUniqueItems(...) helper and remove mechanism from As…

c5bf6e5

…yncBatchingWorkQueue

Clean up ImmutableArrayExtensions

9296580

davidwengier reviewed Mar 25, 2024

View reviewed changes

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared.Test/ImmutableArrayExtensionsTests.cs Outdated Show resolved Hide resolved

davidwengier reviewed Mar 25, 2024

View reviewed changes

Improve test assertion

de084f0

jjonescz reviewed Mar 26, 2024

View reviewed changes

DustinCampbell added 3 commits March 26, 2024 06:36

Fix indentation

357afee

Remove extra word from comment

3667681

Improve test assertion

ab4f568

jjonescz approved these changes Mar 26, 2024

View reviewed changes

DustinCampbell merged commit 3cd3804 into dotnet:main Mar 26, 2024
12 checks passed

DustinCampbell deleted the work-queues branch March 26, 2024 16:20

dotnet-policy-service bot added this to the Next milestone Mar 26, 2024

This was referenced Apr 4, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34803.197 #10218

Closed

[Automated] PRs inserted in VS build main-34803.156 #10219

Closed

[Automated] PRs inserted in VS build feature.dotnetVS-34808.241 #10241

Closed

Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024

DustinCampbell mentioned this pull request Oct 4, 2024

Refresh diagnostics on project changes #10964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `BatchingWorkQueue` with `AsyncBatchingWorkQueue` from Roslyn #10140

Replace `BatchingWorkQueue` with `AsyncBatchingWorkQueue` from Roslyn #10140

DustinCampbell commented Mar 21, 2024 •

edited

Loading

DustinCampbell commented Mar 21, 2024

azure-pipelines bot commented Mar 21, 2024

DustinCampbell commented Mar 25, 2024

davidwengier Mar 25, 2024

DustinCampbell Mar 25, 2024 •

edited

Loading

davidwengier commented Mar 25, 2024

RikkiGibson commented Mar 27, 2024

DustinCampbell commented Mar 27, 2024

davidwengier commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024

RikkiGibson commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024 •

edited

Loading

RikkiGibson commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024

Replace BatchingWorkQueue with AsyncBatchingWorkQueue from Roslyn #10140

Replace BatchingWorkQueue with AsyncBatchingWorkQueue from Roslyn #10140

Conversation

DustinCampbell commented Mar 21, 2024 • edited Loading

DustinCampbell commented Mar 21, 2024

azure-pipelines bot commented Mar 21, 2024

DustinCampbell commented Mar 25, 2024

davidwengier Mar 25, 2024

Choose a reason for hiding this comment

DustinCampbell Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

davidwengier commented Mar 25, 2024

RikkiGibson commented Mar 27, 2024

DustinCampbell commented Mar 27, 2024

davidwengier commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024

RikkiGibson commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024 • edited Loading

RikkiGibson commented Mar 28, 2024

DustinCampbell commented Mar 28, 2024

Replace `BatchingWorkQueue` with `AsyncBatchingWorkQueue` from Roslyn #10140

Replace `BatchingWorkQueue` with `AsyncBatchingWorkQueue` from Roslyn #10140

DustinCampbell commented Mar 21, 2024 •

edited

Loading

DustinCampbell Mar 25, 2024 •

edited

Loading

DustinCampbell commented Mar 28, 2024 •

edited

Loading