Fix codelens crash when a file does not exist #732

rjmholt · 2018-08-23T23:52:58Z

Fixes PowerShell/vscode-powershell#1471 and fixes #718.

If a file is removed when the reference symbols are being parsed, it will cause an exception to be thrown. Now if that file does not exist, we just skip it.

SeeminglyScience · 2018-08-24T11:42:38Z

src/PowerShellEditorServices/Language/LanguageService.cs

                    {
-                        fileMap.Add(file, new ScriptFile(file, null, this.powerShellContext.LocalPowerShellVersion.Version));
+                        scriptFile = new ScriptFile(


Oh? This doesn't try to pull an existing ScriptFile instance? That explains why find references code lens is slow...

Don't know if you want to try this in this PR, but if this could be replaced with Workspace.GetFile() it would speed up finding references significantly. Right now it's only looking at the cache for files that are explicitly referenced (which is pretty rare)

Yeah I was a bit skeptical of that constructor. I'll see if changing it over works

SeeminglyScience · 2018-08-24T11:44:08Z

src/PowerShellEditorServices/Language/LanguageService.cs

+            }
+
+            IEnumerable<string> allFiles = workspace.EnumeratePSFiles();
+            foreach (string file in allFiles)


Possibly inline workspace.EnumeratePSFiles() if allFiles is not used somewhere down the line.

SeeminglyScience · 2018-08-24T11:46:08Z

src/PowerShellEditorServices/Language/LanguageService.cs

-                var allFiles = workspace.EnumeratePSFiles();
-                foreach (var file in allFiles)
+            // We want to look for references first in referenced files, hence we use ordered dictionary
+            var fileMap = new OrderedDictionary(StringComparer.OrdinalIgnoreCase);


I know this is existing code, but wouldn't this be an issue on Linux?

Yeah, this seems suspect...

SeeminglyScience

LGTM. The difference that ScriptFile change will make is nuts. I have no doubt that was a big part of why that code lens is so slow.

Might be worth doing some before and after benchmarks with a workspace that has something massive like the PSAppDeploy Toolkit sitting in it. Would make for some exciting release notes ;)

rkeithhill

LGTM

TylerLeonhardt · 2018-08-27T16:33:33Z

src/PowerShellEditorServices/Language/LanguageService.cs

                    {
-                        fileMap.Add(file, new ScriptFile(file, null, this.powerShellContext.LocalPowerShellVersion.Version));
+                        scriptFile = workspace.GetFile(file);


this is the change that will increase perf, yeah @SeeminglyScience?

Yeah that constructor for ScriptFile (which is now unused, but is public, so...) does a call to File.ReadAllText() -- so it always reads off the file system. I'm always averse to something like touching the filesystem in a constructor (and don't really understand why it's done in this constructor, since there's another one that takes a full string), but the real problem is that it's likely we already have the script file contents loaded, and if we don't, this will cache them too

TylerLeonhardt · 2018-08-27T16:35:41Z

src/PowerShellEditorServices/Language/LanguageService.cs

+                        foundSymbol,
+                        CmdletToAliasDictionary,
+                        AliasToCmdletDictionary)
+                    .Select(reference =>


did you want to try to clean up the LINQ usage here? We can call that out of scope if you want.

~~I'm game!~~

I remember when I hit this before I went and searched if we can avoid the closure allocation by turning this into a static (or instance...) method call, but I didn't find any good information on it... Would be nice if we could retain the readable composability of LINQ

Actually, I'll open a new PR on the LINQ refactoring here, since there are other methods that would benefit. I'll see if I can do a quick benchmark before we merge

TylerLeonhardt

LGTM!

TylerLeonhardt · 2018-08-27T16:36:25Z

Might be worth doing some before and after benchmarks with a workspace that has something massive like the PSAppDeploy Toolkit sitting in it. Would make for some exciting release notes ;)

pls @rjmholt

TylerLeonhardt · 2018-08-27T16:37:35Z

also before this goes in can we merge in the tests PR and rebase from master so CI is happy?

rjmholt · 2018-08-28T02:17:24Z

Just tried this change out with Shift-F12 on this function:
https://github.com/PSAppDeployToolkit/PSAppDeployToolkit/blob/c14bbf373bee4823579c378dc4c55e9d75240313/Sources/BetaToolkit/AppDeployToolkitMain.ps1#L388

Before this change:

Found symbol references for Write-FunctionHeaderOrFooter in /home/rob/Documents/Dev/Microsoft/PSAppDeployToolkit/Sources/BetaToolkit/AppDeployToolkitMain.ps1. [800ms]

After this change:

Found symbol references for Write-FunctionHeaderOrFooter in /home/rob/Documents/Dev/Microsoft/PSAppDeployToolkit/Sources/BetaToolkit/AppDeployToolkitMain.ps1. [76ms]

10x speedup for finding 268 references of a function defined in a 9000 line file. More importantly, doing that before the change makes the whole editor feel very sluggish -- I think that's a consequence of something qualitatively worse than more processing or allocations, like maybe open file handles accruing?

SeeminglyScience · 2018-08-28T02:33:05Z

@rjmholt

More importantly, doing that before the change makes the whole editor feel very sluggish -- I think that's a consequence of something qualitatively worse than more processing or allocations, like maybe open file handles accruing?

Do you have the code lens on? If so, this isn't just running when you manually hit find references. This is running every single document edit. Reparsing all those files just about every key press... general sluggishness is no surprise.

This is going to make a lot of people happy. That difference is incredible.

rjmholt · 2018-08-28T03:21:03Z

Yes CodeLens on and all. Just mean that it's not just CodeLens that sped up, even though that's what I measured; ScriptAnalysis on that file took something insane like 100 seconds, but fixing the CodeLens issue seems to have relieved much more pressure.

rkeithhill · 2018-08-28T04:10:14Z

100 secs on a single file? Yikes!!

rjmholt · 2018-08-28T04:13:32Z

@rkeithhill Yeah, it's not good. We've discussed some ways we might be able to optimise it here:
PowerShell/PSScriptAnalyzer#1056

Robert Holt added 2 commits August 23, 2018 16:44

[Style] Refactor FindReferencesOfSymbol code

5ac8362

Check that file exists before parsing for symbols

700d4f6

rjmholt requested review from TylerLeonhardt, rkeithhill and SeeminglyScience August 23, 2018 23:52

SeeminglyScience reviewed Aug 24, 2018

View reviewed changes

Fix unneccessary file read for workspace script files

5ba4f30

SeeminglyScience approved these changes Aug 24, 2018

View reviewed changes

rkeithhill approved these changes Aug 24, 2018

View reviewed changes

TylerLeonhardt reviewed Aug 27, 2018

View reviewed changes

TylerLeonhardt approved these changes Aug 27, 2018

View reviewed changes

rjmholt mentioned this pull request Aug 27, 2018

Editor services crashing when file in use PowerShell/vscode-powershell#1499

Closed

Fix unsupported API usage in .NET Framework

b5ad8cd

rjmholt merged commit 90999e4 into PowerShell:master Aug 28, 2018

rjmholt mentioned this pull request Sep 18, 2018

IntelliSense slow or unresponsive forced to kill powershell to force it to work. PowerShell/vscode-powershell#1256

Closed

rjmholt deleted the fix-codelens-fileexist-crash branch December 12, 2018 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix codelens crash when a file does not exist #732

Fix codelens crash when a file does not exist #732

rjmholt commented Aug 23, 2018 •

edited

Loading

SeeminglyScience Aug 24, 2018

rjmholt Aug 24, 2018

SeeminglyScience Aug 24, 2018

SeeminglyScience Aug 24, 2018

rjmholt Aug 24, 2018

SeeminglyScience left a comment

rkeithhill left a comment

TylerLeonhardt Aug 27, 2018

rjmholt Aug 27, 2018

TylerLeonhardt Aug 27, 2018

rjmholt Aug 27, 2018 •

edited

Loading

rjmholt Aug 28, 2018

rjmholt Aug 28, 2018

TylerLeonhardt left a comment

TylerLeonhardt commented Aug 27, 2018

TylerLeonhardt commented Aug 27, 2018

rjmholt commented Aug 28, 2018 •

edited

Loading

SeeminglyScience commented Aug 28, 2018

rjmholt commented Aug 28, 2018

rkeithhill commented Aug 28, 2018

rjmholt commented Aug 28, 2018

Fix codelens crash when a file does not exist #732

Fix codelens crash when a file does not exist #732

Conversation

rjmholt commented Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience left a comment

Choose a reason for hiding this comment

rkeithhill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Aug 27, 2018

TylerLeonhardt commented Aug 27, 2018

rjmholt commented Aug 28, 2018 • edited Loading

SeeminglyScience commented Aug 28, 2018

rjmholt commented Aug 28, 2018

rkeithhill commented Aug 28, 2018

rjmholt commented Aug 28, 2018

rjmholt commented Aug 23, 2018 •

edited

Loading

rjmholt Aug 27, 2018 •

edited

Loading

rjmholt commented Aug 28, 2018 •

edited

Loading