-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Improve variable completion performance #19595
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.
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
Improve variable completion performance #19595
Conversation
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/CommandCompletion/CompletionCompleters.cs
Show resolved
Hide resolved
BTW, I agree with the 3 functional changes you called out in the PR description. I think they make sense. |
…letionCompleters.cs Co-authored-by: Dongbo Wang <[email protected]>
…letionCompleters.cs Co-authored-by: Dongbo Wang <[email protected]>
…ontentCmdletProvider
…artinGC94/PowerShell into ImproveVariableCompletionSpeed
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@MartinGC94, thanks for your contribution! |
🎉 Handy links: |
PR Summary
This is a small rewrite of the variable completion code to make it faster.
While the general performance was fine, the first variable tab completion was too slow for me.
On my PC the initial variable completion inside a long script takes about 1.8 seconds, and subsequent attempts take 50ms.
With the new code the first attempt takes about 100ms, and drops down to around 4ms for subsequent attempts.
The logic stays mostly the same, I've simply replaced
Get-Item
calls with the equivalent internal API calls to skip the PS invocation overhead. Most of the performance improvements however come from the updated Astvisitor code. Instead of adding every variable instance to a list and later analyzing the list with a bunch of duplicates, I only look for variable assignments and save the most recent type constraint and static assignment types in a dictionary for later reference.There are some minor functional changes that are worth discussing:
When analyzing script text I don't process variables after the cursor. This seems more accurate but maybe users like having all the script variables show up in the completion, even if they appear after the cursor.
For commands with "OutVariable" I don't bother using the type inference to try and figure out the output type of the command and instead assume it's an arraylist. The reason for this is that PowerShell doesn't unwrap the ArrayLists created by these parameters so it's technically more accurate to list those variables as ArrayLists.
Pipeline variables are only included in the completion result if the cursor is within the pipeline.
PR Context
Like I said, I thought it was a bit too slow.
Also:
Fixes #18688
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).