Some improvement for import completion by genlu · Pull Request #43548 · dotnet/roslyn

genlu · 2020-04-21T23:23:28Z

Added 500ms timebox to completion to unimported extension method
various perf improvements to reduce allocations
Removed text "experimental" from UI, but kept the experiment code. The plan is to disable this via experiment for all external RTW users while enable it for internal and preview users

sharwell · 2020-10-01T23:52:18Z

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

💡 You can pass a length to GetInstance. That may or may not help here.

it just calls EnsureCapacity underneath
http://sourceroslyn.io/#Microsoft.CodeAnalysis/ArrayBuilder.cs,355

CyrusNajmabadi · 2020-10-02T00:45:09Z

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

was this a speculative perf change? or was this driven from some actual data? i have no problem with it either way, i'm just curious.

This is addressing RPS regression caught in validation PR.

Before: +6.3mb LOH allocation
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS/pullrequest/278169

After: +610kb LOH allocation
https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/VS/pullrequest/278694

@sharwell is your sdgmented array ready?

…ion2

sharwell · 2020-10-21T06:16:36Z

Is there a reason we are removing the experiment? I'd like to go over the details with you before making this change.

This reverts commit fad9d46.

This reverts commit bbd312b.

genlu · 2020-10-21T22:35:03Z

@sharwell @CyrusNajmabadi please take a look, thanks

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

CyrusNajmabadi · 2020-10-22T00:56:04Z

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

+                }
+            }
+
+            public IEnumerator<CompletionItem> GetEnumerator()


do you actually enumerate this dictionary? if so, you could consider making a struct enumrator so you save on this allocatoin as well.

yes, but only once (per completion session), so it's probably no t necessary

CyrusNajmabadi · 2020-10-22T00:56:34Z

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

            return hash;
        }

+        private class DisplayNameToItemsMap : IEnumerable<CompletionItem>


considre not directly exposing this as a IEnumerable. will prevent anything from going through teh interface-dispatch pathways here.

What do you mean by this? I'm passing this to ArrayBuilder.AddRange(), so I think the interface is required?

ah ok. another option would be for you to enumerate this yourself, calling ArrayBuilder.Add.

But if this is only one allocation, it's nbd.

CyrusNajmabadi · 2020-10-22T00:58:17Z

...letion/Providers/ImportCompletionProvider/AbstractExtensionMethodImportCompletionProvider.cs

+                    // Either the timebox is not enabled, so we need to wait until the operation for complete,
+                    // or there's no timeout, and we now have all completion items ready.
+                    var items = await getItemsTask.ConfigureAwait(false);
+                    nestedTokenSource.Cancel();


is this cancellation necessary? you only get here if the computation succeeded, so why do we need to cancel anything?

you are right, this can only cancels the Delay task

CyrusNajmabadi · 2020-10-22T00:58:24Z

...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs

            public Checksum Checksum { get; }

            private ImmutableArray<TypeImportCompletionItemInfo> ItemInfos { get; }
+            private int PublicItemCount { get; }


Added doc. We need to exact number of items we are dealing with, since some of the optimization rely on this knowledge to avoid allocation from calls to Resize

CyrusNajmabadi · 2020-10-22T00:58:51Z

...pletion/Providers/ImportCompletionProvider/AbstractTypeImportCompletionService.CacheEntry.cs

                        extensionMethodData: null);

+                    if (isPublic)
+                        _publicItemCount++;


need this all doc'ed. i couldn't really undrestand what was going on.

CyrusNajmabadi · 2020-10-22T00:59:19Z

Looking good overall. Some things i'm not sure i understand just yet.

genlu · 2020-10-23T23:26:43Z

@CyrusNajmabadi Any more comments on this?

genlu · 2020-10-26T18:30:36Z

Ping @CyrusNajmabadi

CyrusNajmabadi · 2020-10-26T18:50:29Z

can try to get to this tomorrow. please pping me then.

genlu · 2020-10-27T22:07:35Z

Ping @sharwell as well

Cosifne · 2020-10-28T00:51:27Z

src/VisualStudio/CSharp/Impl/CSharpVSResources.resx

  </data>
  <data name="Show_items_from_unimported_namespaces" xml:space="preserve">
-    <value>Show items from unimported namespaces (experimental)</value>
+    <value>Show items from unimported namespaces</value>


By removing the 'experiment' flag, does that mean we are plan to use the cancellation data to see if we can enable this by default for all users?

This feature has been stabilized for a few releases, so functionally we think we are ready to move it out of the experimental state. However, current implementation still incurs too much of perf hit for many users to justify enabling it by default. Various things contributed to this, e.g. OOP cost, some existing editor APIs is not designed for handling large amount of items, etc. We will keep making improvement and monitoring the telemetry, hopefully we can turn it on by default in a future release.

Cosifne

I just have a few questions about the user scenarios after this change.
The optimization parts LGTM, (but I don't know how many real benefits it would have). I saw you have posted some memory data around this change. Would it also bring perf benefits? (I am guessing it might help GC)

Port #43548 from master-vs-deps to master

jinujoseph added the Area-IDE label Apr 22, 2020

genlu changed the base branch from master to master-vs-deps September 23, 2020 22:04

genlu changed the base branch from master-vs-deps to master September 23, 2020 22:04

This was referenced Sep 28, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

genlu changed the base branch from master to master-vs-deps October 1, 2020 01:06

sharwell reviewed Oct 1, 2020

View reviewed changes

CyrusNajmabadi reviewed Oct 2, 2020

View reviewed changes

genlu force-pushed the EnableImportCompletion branch from bdacba4 to 1eab87a Compare October 5, 2020 22:54

genlu added 4 commits October 9, 2020 14:05

Perf improvements

127a1de

Remove import completion experiment and enable it by default

bbd312b

Merge remote-tracking branch 'dotnet/master' into EnableImportComplet…

0db127d

…ion2

Add timebox to unimport ext method completion

f713449

genlu force-pushed the EnableImportCompletion branch from 472e00e to f713449 Compare October 20, 2020 00:03

genlu added 2 commits October 20, 2020 22:16

Pool objects to reduce allocation

aa0de62

Disable import completion by default

fad9d46

genlu force-pushed the EnableImportCompletion branch from 855017c to fad9d46 Compare October 21, 2020 05:22

Eliminate call to resize

1e66540

genlu changed the title ~~Enable import completion by default~~ Disable import completion by default Oct 21, 2020

runfoapp bot mentioned this pull request Oct 21, 2020

Roslyn failing to clean up listeners #48816

Closed

genlu added 4 commits October 21, 2020 14:47

Revert "Disable import completion by default"

06e9981

This reverts commit fad9d46.

Revert "Remove import completion experiment and enable it by default"

6b27a67

This reverts commit bbd312b.

Remove the text "experimental" from import completion option

6e8bbdd

Fix test

79e34bf

genlu changed the title ~~Disable import completion by default~~ Some improvement for import completion Oct 21, 2020

genlu marked this pull request as ready for review October 21, 2020 22:33

genlu requested a review from a team as a code owner October 21, 2020 22:33

CyrusNajmabadi reviewed Oct 21, 2020

View reviewed changes

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

More resource pooling to reduce allocation

54107a6

CyrusNajmabadi reviewed Oct 22, 2020

View reviewed changes

genlu added 4 commits October 21, 2020 18:04

Make sure we free pooled resource

f5c0f64

Add comment

e6172e4

Remove unnecessary cancellation

08018a8

Use IDisposable pattern

cf5e2be

This was referenced Oct 22, 2020

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

Fix resx

91cae8a

runfoapp bot mentioned this pull request Oct 23, 2020

OSX deprovision jaredpar/runfo#41

Closed

don't reset capacity for pooled object

8ae23d3

genlu force-pushed the EnableImportCompletion branch from 201c293 to 8ae23d3 Compare October 23, 2020 18:05

Cosifne reviewed Oct 28, 2020

View reviewed changes

Cosifne approved these changes Oct 28, 2020

View reviewed changes

genlu merged commit 3cf59c9 into dotnet:master-vs-deps Oct 28, 2020

ghost added this to the Next milestone Oct 28, 2020

genlu mentioned this pull request Oct 28, 2020

Port #43548 from master-vs-deps to master #48987

Merged

ghost pushed a commit that referenced this pull request Oct 28, 2020

Merge pull request #48987 from genlu/EnableImportCompletion

a13ee2c

Port #43548 from master-vs-deps to master

genlu deleted the EnableImportCompletion branch October 28, 2020 21:02

allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020

Conversation

genlu commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell Oct 1, 2020 • 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!

sharwell commented Oct 21, 2020

Uh oh!

genlu commented Oct 21, 2020

Uh oh!

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!

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!

CyrusNajmabadi commented Oct 22, 2020

Uh oh!

genlu commented Oct 23, 2020

Uh oh!

genlu commented Oct 26, 2020

Uh oh!

CyrusNajmabadi commented Oct 26, 2020

Uh oh!

genlu commented Oct 27, 2020

Uh oh!

Cosifne Oct 28, 2020 • 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!

Cosifne left a comment • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

genlu commented Apr 21, 2020 •

edited

Loading

sharwell Oct 1, 2020 •

edited

Loading

Cosifne Oct 28, 2020 •

edited

Loading

Cosifne left a comment •

edited

Loading