Compute PatternMatching result in parallel when refreshing completion list #67249

genlu · 2023-03-09T22:57:25Z

The change is in 2f70efd. First 3 commits are from #67232, which is needed for this change.

I have compared 4 different implementation for pattern matching locally:

Current is the current implmentation
PLinq without merging (implementation used in this PR)

                    Enumerable.Range(0, _snapshotData.InitialSortedItemList.Count)
                        .AsParallel()
                        .WithCancellation(cancellationToken)
                        .ForAll(CreateMatchResult);   // using lock to add to shared list as part of `CreateMatchResult`

PLinq with merging

                    var results = Enumerable.Range(0, _snapshotData.InitialSortedItemList.Count)
                        .AsParallel()
                        .WithMergeOptions(ParallelMergeOptions.AutoBuffered)
                        .WithCancellation(cancellationToken)
                        .Select(CreateMatchResult);   // no lock, return PatternMatch instead

                    foreach (var result in results)
                    {
                        if (result is not null)
                            list.Add(result.Value);
                    }

Linq

                    var results = Enumerable.Range(0, _snapshotData.InitialSortedItemList.Count).Select(CreateMatchResult);
                    foreach (var result in results)
                    {
                        if (result is not null)
                            list.Add(result.Value);
                    }

My machine has 16 logical processors, I have observed all 16 being utilized when PLinq is used. The test scenario has 29k items to process, which is about 97th percentile of item counts based on telemetry. Here's the average result (in milliseconds):

When filter text is not empty (i.e. actually calls PatternMatcher)

implementation	pattern matching cost	sorting cost
Current	53	9
PLinq without merging	9	10
PLinq with merging	9	10
Linq	56	8

When filter text is empty (i.e. no call to PatternMatcher)

implementation	pattern matching cost	sorting cost
Current	6	5
PLinq without merging	6	8
PLinq with merging	7	10
Linq	8	5

Fix caching of PatternMatchers Deleted CompletionHelperService

CyrusNajmabadi · 2023-03-09T23:14:16Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

@@ -128,20 +132,24 @@ private bool ShouldSelectSuggestionItemWhenNoItemMatchesFilterText
                // since the completion list could be long with import completion enabled.
                var itemsToBeIncluded = s_listOfMatchResultPool.Allocate();
                var cancellationTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
+                var threadLocalPatternMatchHelper = new ThreadLocal<PatternMatchHelper>(() => new PatternMatchHelper(_filterText), trackAllValues: true);


what does 'trackAllValues' do?

looking at the docs, unclear why we're setting that to true. can you doc in the code why?

also, doc in the code why we're using a ThreadLocal here (specifically that PatternMatchHelper is not threadsafe, so we want a dedicated one per thread that is active doing the filtering.

CyrusNajmabadi · 2023-03-09T23:16:20Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                Enumerable.Range(0, _snapshotData.InitialSortedItemList.Count)
+                    .AsParallel()
+                    .WithCancellation(cancellationToken)
+                    .ForAll(CreateMatchResultAndProcessMatchingDefaults);


curious why .ForAll, instead of .Where. Then we don't need a lock, right?

Yes, you are right. I saw in my local measurement ForAll has small (but largely negligible) edge over Where, and since ForAll uses no buffer, I assume there would be less allocation (unconfrimed) That's why I picked ForAll.

CyrusNajmabadi · 2023-03-09T23:16:44Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+
+                    // Dispose PatternMatchers
+                    foreach (var helper in threadLocalPatternMatchHelper.Values)
+                        helper.Dispose();


ah. interesting. you use it for disposal. interesting!

CyrusNajmabadi · 2023-03-09T23:17:04Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

+                    foreach (var helper in threadLocalPatternMatchHelper.Values)
+                        helper.Dispose();
+
+                    threadLocalPatternMatchHelper.Dispose();


can you just place the threadLocalPatternMatchHelper in a using block instead?

CyrusNajmabadi · 2023-03-09T23:18:01Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

-                        roslynInitialTriggerKind, roslynFilterReason, _recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, currentIndex,
-                        out var matchResult))
+                    if (threadLocalPatternMatchHelper.Value!.TryCreateMatchResult(itemData.RoslynItem, roslynInitialTriggerKind, roslynFilterReason,
+                        _recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, index, out var matchResult))


Suggested change

_recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, index, out var matchResult))

_recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, index, out var matchResult))

CyrusNajmabadi · 2023-03-09T23:19:07Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

                    {
-                        list.Add(matchResult);
+                        lock (_gate)
+                            list.Add(matchResult);


don't love that inside the parallel we have to lock just to append to a list. would plinq's Where/Select methods (https://learn.microsoft.com/en-us/dotnet/api/system.linq.parallelenumerable.where?view=net-7.0) method work better? then you can just enumerate over those results that succeeded.

CyrusNajmabadi · 2023-03-09T23:19:28Z

src/EditorFeatures/Test2/IntelliSense/CompletionRulesTests.vb

@@ -56,34 +56,39 @@ Namespace Microsoft.CodeAnalysis.Editor.UnitTests.IntelliSense
            Dim culture = New CultureInfo("tr-TR", useUserOverride:=False)

            Dim workspace = New TestWorkspace
-            Dim helper = New CompletionHelper(isCaseSensitive:=False)
+            Dim helper = New PatternMatchHelper(pattern)


CyrusNajmabadi · 2023-03-09T23:19:33Z

src/EditorFeatures/Test2/IntelliSense/CompletionRulesTests.vb

        End Sub

        Private Shared Sub TestNotMatches(pattern As String, wordsToNotMatch() As String)
            Dim culture = New CultureInfo("tr-TR", useUserOverride:=False)
            Dim workspace = New TestWorkspace
-            Dim helper = New CompletionHelper(isCaseSensitive:=True)
+            Dim helper = New PatternMatchHelper(pattern)


CyrusNajmabadi · 2023-03-09T23:20:45Z

src/Features/Core/Portable/Completion/PatternMatchHelper.cs

+                    initialTriggerKind == CompletionTriggerKind.Invoke ||
+                    initialTriggerKind == CompletionTriggerKind.Deletion;
+            }
+        }


was all this code basically moved unchanged?

CyrusNajmabadi · 2023-03-09T23:21:00Z

src/Features/Core/Portable/Completion/PatternMatchHelper.cs

+
+        public void Dispose()
+        {
+            lock (_gate)


probably don't need this gate... but i'm not going to complain if you keep it.

CyrusNajmabadi · 2023-03-09T23:22:15Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.CompletionListUpdater.cs

                {
-                    cancellationToken.ThrowIfCancellationRequested();


i would not mind in a followup pr if we basically tried to be much clearer about setting up parallel state, and what that state mutated. e.g. by extracting a helper function and passing in things like the collections it is expected to mutate. it's pretty hard to reason about things here given how complex this is and how much state is implicitly captured.

arunchndr · 2023-03-10T00:12:48Z

This looks great, thanks for the fix @genlu! Would we be able to tweak completion speedometer prior to reflect the impact?

genlu · 2023-03-10T00:12:53Z

@CyrusNajmabadi I will address the comments in a follow up PR. I'm planning to add more parallelization for filters and highlighting. Thanks

genlu added 4 commits March 7, 2023 17:05

Move code

cbca514

Refactor CompletionHelper

33601fc

Fix caching of PatternMatchers Deleted CompletionHelperService

Fix NRT warning

f323a3b

Calculate matching items in parallel

2f70efd

genlu requested a review from a team as a code owner March 9, 2023 22:57

dotnet-issue-labeler bot added the Area-IDE label Mar 9, 2023

CyrusNajmabadi reviewed Mar 9, 2023

View reviewed changes

CyrusNajmabadi approved these changes Mar 9, 2023

View reviewed changes

genlu enabled auto-merge March 10, 2023 00:18

genlu merged commit 1bc5258 into dotnet:main Mar 10, 2023

ghost added this to the Next milestone Mar 10, 2023

genlu mentioned this pull request Mar 10, 2023

Don't show expanded items in certain cases #67049

Merged

genlu deleted the PLinqItemManager branch March 10, 2023 17:59

allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute PatternMatching result in parallel when refreshing completion list #67249

Compute PatternMatching result in parallel when refreshing completion list #67249

genlu commented Mar 9, 2023 •

edited

Loading

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

genlu Mar 9, 2023 •

edited

Loading

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

CyrusNajmabadi Mar 9, 2023

arunchndr commented Mar 10, 2023

genlu commented Mar 10, 2023

	_recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, index, out var matchResult))
	_recentItemsManager.GetRecentItemIndex(itemData.RoslynItem), _highlightMatchingPortions, index, out var matchResult))

Compute PatternMatching result in parallel when refreshing completion list #67249

Compute PatternMatching result in parallel when refreshing completion list #67249

Conversation

genlu commented Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunchndr commented Mar 10, 2023

genlu commented Mar 10, 2023

genlu commented Mar 9, 2023 •

edited

Loading

genlu Mar 9, 2023 •

edited

Loading