PowerShellEditorServices icon indicating copy to clipboard operation
PowerShellEditorServices copied to clipboard

Find reference code lens results should be cached

Open SeeminglyScience opened this issue 7 years ago • 7 comments

Right now the references code lens searches the AST of every file in the work space for references every document edit. This causes intellisense to crawl in workspaces with a lot of PowerShell files.

We should cache the commands each workspace file references until they are changed.

SeeminglyScience avatar Jun 05 '18 22:06 SeeminglyScience

In terms of implementation, I can imagine us caching the AST of files (a table of filepaths => ASTs) pretty easily. But I see you've written command caching. Do you mean we should cache the commands once they're parsed (as AST subunits)?

If so, would it make sense for us to do the full AST caching and then process that to create a command cache and make invalidation work backwards (so changing a file invalidates the associated AST, which in turn bubbles up to the command cache)?

rjmholt avatar Jun 05 '18 22:06 rjmholt

I was under the impression we were already caching the AST. I thought we stored them as ScriptFile's and then just searched the AST with an AstVisitor each edit. If that is the case then we would only need to add a cache of what symbols it references. We'd have to change the visitor to find all symbol references instead of a specific symbol reference. That may be another performance save though, as right now we search the AST once for every lens.

If I'm wrong and we don't do that, then yes absolutely we should cache the AST asap. That'd be a huge performance difference especially around DSC.

The way I picture it being implemented is as a lazy property on ScriptFile. Then we could just clear it when ScriptFile.ApplyChange is invoked. If we aren't caching the ScriptFile either, that'd probably be step one.

SeeminglyScience avatar Jun 05 '18 22:06 SeeminglyScience

Assigning this to Rob who's going to investigate this next week

TylerLeonhardt avatar Jun 07 '18 21:06 TylerLeonhardt

Had a brief look at the code for AST manipulation here.

Can't find any evidence that we're doing any caching at all.

Very much looking forward to adding some! 😈

rjmholt avatar Jun 08 '18 00:06 rjmholt

Ah it looks like files are cached using Workspace.cs.

rjmholt avatar Jun 08 '18 00:06 rjmholt

I'm going to fully flesh out what I mean here as it can be kind of confusing to visualize.

Lets say you have three files.

Script A

Get-ChildItem
Invoke-MyCommand

Script B

Get-Process
Invoke-MyCommand2

Script C

function Invoke-MyCommand { }

function Invoke-MyCommand2 { }

Now lets say you have Script C open. The way code lens current works is like this:

  1. VSCode says "give me code lenses for Script C"
  2. PSES sees two function definitions to build references for
  3. PSES starts with Invoke-MyCommand, it enumerates the files in the workspace, and scans the AST of all files for references to Invoke-MyCommand.
  4. PSES moves on to Invoke-MyCommand2, it then enumerates the files in the workspace again and rescans the AST of all files for references to Invoke-MyCommand.

Here's how I propose that it works:

  1. VSCode says "give me code lenses for Script C"
  2. PSES sees two function definitions to build references for
  3. PSES starts with Invoke-MyCommand. It enumerates the cached ScriptFile objects in the workspace and does the following for each file.
    1. Checks a lazy property that holds every referenced command. We don't want to build this property every change because it may never be requested.
    2. If the lazy property is null, search the AST of the ScriptFile for every CommandAst and store the result of CommandAst.GetCommandName() and store it as a HashSet<> (warning: result can be null)
    3. If the lazy property is not null, check to see if Invoke-MyCommand is in the HashSet<>
  4. The last step is repeated for Invoke-MyCommand2, but this time all the lazy properties should be filled.

That way every AST is only searched once per change of that file specifically. Right now every AST is searched once for every function definition in the open file, every time that file is changed.

SeeminglyScience avatar Jun 12 '18 11:06 SeeminglyScience

Fixed by #1917 ?

fflaten avatar Feb 07 '23 18:02 fflaten