Skip to content
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

Find reference code lens results should be cached #678

Open
SeeminglyScience opened this issue Jun 5, 2018 · 7 comments
Open

Find reference code lens results should be cached #678

SeeminglyScience opened this issue Jun 5, 2018 · 7 comments
Labels

Comments

@SeeminglyScience
Copy link
Collaborator

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.

@rjmholt
Copy link
Contributor

rjmholt commented Jun 5, 2018

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)?

@SeeminglyScience
Copy link
Collaborator Author

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.

@TylerLeonhardt
Copy link
Member

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

@rjmholt
Copy link
Contributor

rjmholt commented Jun 8, 2018

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
Copy link
Contributor

rjmholt commented Jun 8, 2018

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

@SeeminglyScience
Copy link
Collaborator Author

SeeminglyScience commented Jun 12, 2018

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.

@fflaten
Copy link
Contributor

fflaten commented Feb 7, 2023

Fixed by #1917 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants