Switch AsyncCompletion to new pooled CompletionItemList based APIs by genlu · Pull Request #61477 · dotnet/roslyn

genlu · 2022-05-24T00:33:14Z

LOH allocation for CompletionItem[] and CompletionItemWithHighlight[] are gone.

this will break our CI integration tests, so I queued DartLab integration test run with latest VS build which passed
https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=6272608&view=results

davidwengier · 2022-05-24T02:36:49Z

src/EditorFeatures/Core.Cocoa/Microsoft.CodeAnalysis.EditorFeatures.Cocoa.csproj

         referencing what we need out of the macos SDK. -->
    <PackageDownload Include="Microsoft.macOS.Ref" Version="[$(MicrosoftmacOSRefVersion)]" />
-    <Reference Include="$(NuGetPackageRoot)microsoft.macos.ref\$(MicrosoftmacOSRefVersion)\ref\net6.0\Xamarin.Mac.dll" />
+    <Reference Include="$(NuGetPackageRoot)microsoft.macos.ref\$(MicrosoftmacOSRefVersion)\ref\net6.0\Microsoft.macOS.dll" />


@sandyarmstrong this is (one of) the same change as you're making in #61250

Is this a problem? Or do we need to wait for the 6.0.300 bump in VS Mac?

Ping @sandyarmstrong, could you please take a look at this and let me know if it's OK? Thanks

You should merge #61250 first. But then we will not be able to insert Roslyn with this change for at least a few days while we finish up the work to bump VSMac to use the 6.0.300 SDK and the newer macos workload.

But then we will not be able to insert Roslyn with this change for at least a few days

@davidwengier Would this be a problem in case we need to merge this sooner?

I merged #61250, we should be ready to consume it in VSMac in the next few days.

This reverts commit c44e868.

genlu · 2022-06-06T20:04:05Z

The referenced version of editor package in latest commit is pending insertion
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS/pullrequest/403261

CyrusNajmabadi · 2022-06-09T19:01:38Z

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

            }
        }
+
+        private static SegmentedList<VSCompletionItem> SortCompletionitems(IAsyncCompletionSession session, AsyncCompletionSessionInitialDataSnapshot data)


ideally move this next to the caller.

CyrusNajmabadi · 2022-06-09T19:02:04Z

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

+            return items;
+        }
+
+        private sealed class VSItemComparer : IComparer<VSCompletionItem>


would it be possible for us to jsut expose this IComparer to the caller so that they can own smart data management and sorting? we jsut own comparing?

Yea, sounds good to me, I will bring this up with Editor team.

what's the result of this talk?

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

CyrusNajmabadi · 2022-06-09T19:02:52Z

Done with pass :)

…nSource.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

src/EditorFeatures/Core.Cocoa/Preview/PreviewPane.cs

genlu · 2022-06-10T18:41:58Z

@CyrusNajmabadi PTAL, thanks!

CyrusNajmabadi · 2022-06-10T18:46:16Z

Integration tests seem to be totally broken. :(

CyrusNajmabadi · 2022-06-10T18:47:43Z

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/CompletionSource.cs

-                itemsBuilder.AddRange(context1.Items);
-                itemsBuilder.AddRange(context2.Items);
-
+                var completionList = session.CreateCompletionList(context1.ItemList.Concat(context2.ItemList));


which .Concat is this? is it a single alloc that just streams over both lists? or is it an O(N) alloc that copies everything into a new list?

I think this is the one streams over both lists w/o copying (System.Linq.Enumerable.Concat)

CyrusNajmabadi · 2022-06-10T18:49:24Z

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

-                            itemsBuilder.AddRange(data.InitialSortedList);
-                            itemsBuilder.AddRange(expandedContext.Items);
-                            var combinedList = itemsBuilder.MoveToImmutable();
+                            var combinedItemList = session.CreateCompletionList(data.InitialSortedItemList.Concat(expandedContext.ItemList));


same question about this concat. is it a single-alloc? or an O(n) copy?

CyrusNajmabadi · 2022-06-10T18:52:05Z

CompletionList is now:

    public sealed class CompletionList<T> : IReadOnlyList<T>, IDisposable

Who is expected to dispose this.

Note: calling this CompletionList was very confusing as i thought that meant it was a list of completoin items. When really it's just a list of anything, it's just used in completion.

genlu · 2022-06-10T19:14:19Z

Who is expected to dispose this.

IT's associated to a async completion session, and Editor will free them after the session is done (dismissed/committed/etc)

olegtk · 2022-06-10T20:59:10Z

I agree CompletionList<T> is somewhat confusing name. It reflects existing state of completion API where completion items are passed as ImmutableArray<CompletionItem> and also ImmutableArray<CompletionItemWithHighlight> and CompletionItem and CompletionItemWithHighlight types do not have common supertype...
The only way to create CompletionList<T> is through IAsyncCompletionSession though, so that somewhat makes it a bit clearer when used.

CyrusNajmabadi · 2022-06-10T22:52:36Z

I'm pretty ok with teh code. But i would like some sort of integration test pass to validate that this isn't going to break things for some unforseen reason. Thanks!

CyrusNajmabadi · 2022-06-13T19:30:08Z

Awesome!

ghost added the Area-IDE label May 24, 2022

genlu requested a review from olegtk May 24, 2022 00:48

davidwengier reviewed May 24, 2022

View reviewed changes

genlu added 2 commits May 27, 2022 18:03

Update to new editotr package

e29be48

Fix error after upgrade

4eda0c1

genlu force-pushed the NewEditor branch 2 times, most recently from d7f1e17 to 46d939c Compare May 31, 2022 21:17

Switch to allocation free AsyncCompletion API

c0a9e57

genlu force-pushed the NewEditor branch from 46d939c to 033673d Compare June 1, 2022 00:35

genlu mentioned this pull request Jun 1, 2022

Reducing allocations in completion #61620

Merged

genlu added 2 commits June 1, 2022 17:00

Fix

b0a9ffa

Update editor package

27a5ae5

genlu force-pushed the NewEditor branch from 033673d to 27a5ae5 Compare June 2, 2022 00:01

genlu changed the base branch from main to main-vs-deps June 2, 2022 00:16

genlu added 3 commits June 1, 2022 17:19

Merge remote-tracking branch 'dotnet/main-vs-deps' into NewEditor

96d9a34

Fix test

c44e868

Merge remote-tracking branch 'origin/NewEditor' into NewEditor

ed4c1bf

genlu marked this pull request as ready for review June 2, 2022 20:37

genlu requested a review from a team as a code owner June 2, 2022 20:37

genlu requested a review from a team June 2, 2022 20:37

genlu requested a review from a team as a code owner June 2, 2022 20:37

Revert "Fix test"

12fcecf

This reverts commit c44e868.

Update Editor packages

b2271f6

genlu force-pushed the NewEditor branch from 667e7ef to b2271f6 Compare June 6, 2022 21:12

CyrusNajmabadi reviewed Jun 9, 2022

View reviewed changes

src/EditorFeatures/Core/IntelliSense/AsyncCompletion/ItemManager.cs Show resolved Hide resolved

genlu and others added 4 commits June 9, 2022 12:09

Update src/EditorFeatures/Core/IntelliSense/AsyncCompletion/Completio…

a279562

…nSource.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Update src/EditorFeatures/Core/IntelliSense/AsyncCompletion/Completio…

794174f

…nSource.cs Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>

Address comments

b2008de

Surpress obsolete warnings

c6af434

sandyarmstrong reviewed Jun 9, 2022

View reviewed changes

src/EditorFeatures/Core.Cocoa/Preview/PreviewPane.cs Outdated Show resolved Hide resolved

Fix

2032a13

genlu force-pushed the NewEditor branch from 76309ec to 2032a13 Compare June 9, 2022 21:45

CyrusNajmabadi reviewed Jun 10, 2022

View reviewed changes

CyrusNajmabadi approved these changes Jun 13, 2022

View reviewed changes

Merge remote-tracking branch 'dotnet/main-vs-deps' into NewEditor

b3d5370

genlu enabled auto-merge June 13, 2022 18:01

genlu merged commit e23d497 into dotnet:main-vs-deps Jun 13, 2022

ghost added this to the Next milestone Jun 13, 2022

genlu deleted the NewEditor branch June 13, 2022 19:14

vatsalyaagrawal added Approved to merge and removed Needs Shiproom Approval labels Jun 23, 2022

RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022

Conversation

genlu commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidwengier May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu commented Jun 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 9, 2022

Uh oh!

Uh oh!

genlu commented Jun 10, 2022

Uh oh!

CyrusNajmabadi commented Jun 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Jun 10, 2022

Uh oh!

genlu commented Jun 10, 2022

Uh oh!

olegtk commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jun 10, 2022

Uh oh!

CyrusNajmabadi commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

genlu commented May 24, 2022 •

edited

Loading

davidwengier May 24, 2022 •

edited

Loading

olegtk commented Jun 10, 2022 •

edited

Loading