Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

TylerLeonhardt · 2019-03-22T04:15:38Z

In PSScriptAnalyzer 1.18, they've added parse errors to the diagnostic records that get sent back from PSSA.

This broke us because they come in as a different ScriptFileMarkerLevel called ParseError that we didn't handle.

This PR adds that additional enum member and filters PSSA invocations to only get back all diagnostics other than ParseErrors. PSES still handles its own ParseErrors so that (1) there's no startup cost to ParseErrors, (2) ParseErrors get shown no matter whether PSSA is available.

cc @bergmeister

rkeithhill · 2019-03-22T04:52:06Z

I'll try to fully review this tomorrow but one concern I have is that if someone turns off codeAnalysis, they'll lose parse errors now, right? Or does this fallback to using the PSES parse error diagnostics if code analysis is disabled. Oh yeah, maybe two concerns - PSSA performance - I get really tired of waiting for PSSA diagnostic markers catching up with my code edits in large files. Squiggles often display on the wrong lines for minutes some times. I'd hate to not get updated on parse errors for minutes. That could mean I've committed a file with a parse error. :-(

rkeithhill · 2019-03-22T04:53:57Z

Now that I think about, I'd almost prefer that we disable that rule (or ignore the marker) and rely on our parse error diagnostics because performance. I timed v1.18 on the InvokePlaster.ps1 file and it still takes 15 secs to complete. Seems like the response time on parse errors should be much faster than that.

bergmeister · 2019-03-22T13:59:19Z

src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs

+            {
+                throw new ArgumentException(
+                string.Format(
+                        "The provided DiagnosticSeverity value '{0}' is unknown.",


Minor nit: Using string interpolation ($"") we could merge 3 lines into 1 whilst having better readability.

bergmeister · 2019-03-22T14:11:41Z

@rkeithhill The new version should be twice as fast for cold starts and magnitudes faster when re-analyzing. I am not sure if this is a PSES or PSSA issue that refreshes sometimes take a long time or require more typing.
I will take a look at your file and see if it is somehow special.
On my machine (4 core laptop i7), I can analyse the build.psm1 of the PowerShell/Power repo in less than 10 seconds and this file has thousands of lines of code now.
I also have a PR open for 1.19 to halve execution time further for cold starts btw.

JamesWTruher · 2019-03-22T16:56:03Z

@rkeithhill was that 15-second delay a cold start, or every time you pressed a key? If it was just cold-start, how would you characterize performance after cold-start?

JamesWTruher · 2019-03-22T17:10:26Z

I did a quick check on my mac with 1.18, cold-start definitely takes longer than I like, here is what I saw:
(cold-start, followed by warm-start)

PS> measure-command { invoke-scriptanalyzer ./InvokePlaster.ps1 }
Seconds           : 8
Milliseconds      : 889
Ticks             : 88895066
TotalSeconds      : 8.8895066
TotalMilliseconds : 8889.5066

PS> measure-command { invoke-scriptanalyzer ./InvokePlaster.ps1 }
Seconds           : 0
Milliseconds      : 68
Ticks             : 681868
TotalSeconds      : 0.0681868
TotalMilliseconds : 68.1868

bergmeister · 2019-03-22T17:30:25Z

I did a comparison of PSSA 1.17.1 and 1.18.0 and the time (cold-start) went down from 28.7 to 14.3 seconds, thereby proving what we promised with the new release. Furthermore the time of a re-run of the same file went down from 11 to 0.1 seconds.
The following PR can nearly halve the time for cold-runs further to 8.9 seconds: PowerShell/PSScriptAnalyzer#1178
The bottleneck for the analysis of cold runs are the calls to Get-Command that 5 rules make and also how slow the slowest rule is (the above PR speeds up the slowest rule). I once did an experiment to just initialise the CommandInfo cache once in a background thread by calling Get-Command once but due this PowerShell issue, I could not use the results: PowerShell/PowerShell#8910

rkeithhill · 2019-03-22T17:33:22Z

Fair point, my second invocation does drop to < 1 second.

How does your mac behave (what's the CPU usage) while this runs:

for ($i = 0; $i -lt 100; $i++) { Measure-Command { Invoke-ScriptAnalyzer -Settings ..\ScriptAnalyzerSettings.psd1 -Path .\InvokePlaster.ps1 }; Start-Sleep -Milliseconds 250 }

This simulates someone typing in a commandline of 100 chars where keystrokes are spaced about 250 milliseconds apart (OK, they are a fast-ish typist). What I see on my laptop (apparently twice as slow as your mac) is that while this runs, my CPU sits at 10%. That's not horrible but not great either.

That said, we do have code in the extension that anticipates this scenario (delayThenInvokeDiagnostics) that should help here. Anyway, rather that characterize perf in isolation, I think we need to eval the perf of this running in the extension on some good size files and not only loading but editing them to see what the experience is like. Keeping track of how long a parse error diag takes to show up and what the CPU usage is. We've already gotten complaints about CPU usage.

rkeithhill · 2019-03-22T17:38:44Z

BTW the perf work & results on PSSA are great! Great job there. It's just that use with the extension is perhaps a bit abusive (runs a lot). Also we have other issues with the the message processing getting behind due to its sequential nature and the fact that we dont' handle cancelRequests. I have a PR open to attempt to address the cancelRequests but it hasn't gotten any traction yet.

TylerLeonhardt · 2019-03-22T17:40:14Z

@JamesWTruher and I talked about this and agree with @rkeithhill in having PSES handle the parse errors... but we agree for a different reason.

One of the things @rjmholt, @JamesWTruher and I have been set out to do was switch to using PSSA via a C# API that @JamesWTruher has been working on (not quite done yet).

One of the requirements for that work is the ability to hand PSSA an AST so that the rules can be run on that because PSES already has the AST.

If you pass something the AST, it can't get the parse errors so PSES will have to send parse errors from the initial parsing of the file anyway.

I'll filter out the ParseErrors for now and continue sending them via PSES in preparation for PSSA as an API that accepts an AST.

rkeithhill · 2019-03-22T17:42:45Z

Ooh, I like the idea of using the API directly. That cuts out a bunch of PowerShell invocation goo and redundant parsing! Plus doing it this way means we still get parse error diags even if someone chooses to disable code analysis.

bergmeister · 2019-03-22T18:09:41Z

Yes, the high level plan is to publish a NuGet package of PSSA (Engine and Rules) that multi-targets net452 and netstandard2.0. Due to conditional compilation we will have to drop support for PS version 3 and 4 though. Then PSES could reference this package (and also reduce code duplication) and call into the API's directly instead of calling the cmdlets via PowerShell. In the past folks have reported memory issue though but I hope this should not happen any more. I ran PSSA against the whole test folder of the PowerShell repo, which results in 800MB of memory usage but it does not grow when re-analysing (I am calling [GC}::Collect(). I suspect the size is mainly due to CommandInfo being a bit wasteful with memory similar to PSObject.
With the NuGet package, people can still install different version of PSSA and there is less coupling (we could release updates to the NuGet package to the extension first to test out coming changes before publishing PSSA

TylerLeonhardt · 2019-03-22T18:42:33Z

I've updated the PR to use parse errors from PSES.

rkeithhill · 2019-03-22T19:07:03Z

src/PowerShellEditorServices/Workspace/ScriptFileMarker.cs

@@ -162,36 +164,27 @@ internal static ScriptFileMarker FromDiagnosticRecord(PSObject psObject)
                };
            }

+            string severity = diagnosticRecord.Severity.ToString();
+            if(!Enum.TryParse(severity, out ScriptFileMarkerLevel level))


Minor nit, space between if and opening (.

I'm glad I wasn't the only one that felt like this

Good catch - fixed.

rkeithhill · 2019-03-22T19:16:31Z

src/PowerShellEditorServices.Protocol/Server/LanguageServer.cs

                }

+                scriptFile.SyntaxMarkers.AddRange(semanticMarkers);


Do we really want to modify the SyntaxMarkers collection in ScriptFile by adding semantic markers? Maybe we should have two collections in ScriptFile (SyntaxMarkers and SemanticMarkers) or perhaps rename the property to DiagnosticMarkers (or something like that).

I went with the rename since the syntax markers and semantic markers are both treated the same when they get sent to vscode.

rkeithhill

LGTM

rjmholt · 2019-03-22T22:59:01Z

src/PowerShellEditorServices/Analysis/AnalysisService.cs

@@ -55,7 +55,7 @@ public class AnalysisService : IDisposable
        /// <summary>
        /// An empty script marker result to return when no script markers can be returned.
        /// </summary>
-        private static readonly ScriptFileMarker[] s_emptyScriptMarkerResult = new ScriptFileMarker[0];
+        private static readonly List<ScriptFileMarker> s_emptyScriptMarkerResult = new List<ScriptFileMarker>();


If this is supposed to be a readonly empty value, it can't be a List<T>.

Otherwise, if we need to return a mutable collection, we should either:

Return a readonly collection here and create a new mutable collection around it in the caller if it needs extending, or;

Return a List and not have an s_emptyScriptMarkerResult, but instead return a new List<T>() every time.

If we return s_emptyScriptMarkerResult and the caller then mutates it, those additions will become permanently added to s_emptyScriptMarkerResult.

good point. I've switched to just some new lists.

…erLevel and only have it send parse errors

…d only have it send parse errors (#891)

Add new PSSA 1.18 ParseError Severity

b8f56fa

TylerLeonhardt requested review from rkeithhill, rjmholt, JamesWTruher and SeeminglyScience March 22, 2019 04:15

TylerLeonhardt changed the title ~~Add new parseerror sev~~ Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors Mar 22, 2019

bergmeister approved these changes Mar 22, 2019

View reviewed changes

ignore parseerrors from PSSA

4937f56

TylerLeonhardt force-pushed the add-new-parseerror-sev branch from 6c3af28 to 4937f56 Compare March 22, 2019 18:41

rkeithhill reviewed Mar 22, 2019

View reviewed changes

TylerLeonhardt added 2 commits March 22, 2019 13:22

address feedback

a81d4ae

back to empty list since we supply parse errors in PSES again

0e05c92

rkeithhill approved these changes Mar 22, 2019

View reviewed changes

TylerLeonhardt added the Backport-Legacy label Mar 22, 2019

rjmholt suggested changes Mar 22, 2019

View reviewed changes

address feedback

a57fbd0

rjmholt approved these changes Mar 23, 2019

View reviewed changes

TylerLeonhardt merged commit 2f79bcf into PowerShell:master Mar 25, 2019

TylerLeonhardt deleted the add-new-parseerror-sev branch March 25, 2019 18:57

TylerLeonhardt added a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Mar 25, 2019

cherry pick PowerShell#888 Add new ParseError level to ScriptFileMark…

e203de7

…erLevel and only have it send parse errors

TylerLeonhardt mentioned this pull request Mar 25, 2019

cherry pick #888 Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #891

Merged

TylerLeonhardt added a commit that referenced this pull request Mar 26, 2019

cherry pick #888 Add new ParseError level to ScriptFileMarkerLevel an…

3c6b478

…d only have it send parse errors (#891)

TylerLeonhardt added Backport-Complete and removed Backport-Legacy labels Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

TylerLeonhardt commented Mar 22, 2019 •

edited by rjmholt

Loading

rkeithhill commented Mar 22, 2019

rkeithhill commented Mar 22, 2019 •

edited

Loading

bergmeister Mar 22, 2019

bergmeister commented Mar 22, 2019 •

edited

Loading

JamesWTruher commented Mar 22, 2019 •

edited

Loading

JamesWTruher commented Mar 22, 2019

bergmeister commented Mar 22, 2019 •

edited

Loading

rkeithhill commented Mar 22, 2019 •

edited

Loading

rkeithhill commented Mar 22, 2019 •

edited

Loading

TylerLeonhardt commented Mar 22, 2019

rkeithhill commented Mar 22, 2019

bergmeister commented Mar 22, 2019

TylerLeonhardt commented Mar 22, 2019

rkeithhill Mar 22, 2019

bergmeister Mar 22, 2019

TylerLeonhardt Mar 22, 2019

rkeithhill Mar 22, 2019

TylerLeonhardt Mar 22, 2019

rkeithhill left a comment

rjmholt Mar 22, 2019

TylerLeonhardt Mar 22, 2019

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

Add new ParseError level to ScriptFileMarkerLevel and only have it send parse errors #888

Conversation

TylerLeonhardt commented Mar 22, 2019 • edited by rjmholt Loading

rkeithhill commented Mar 22, 2019

rkeithhill commented Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

bergmeister commented Mar 22, 2019 • edited Loading

JamesWTruher commented Mar 22, 2019 • edited Loading

JamesWTruher commented Mar 22, 2019

bergmeister commented Mar 22, 2019 • edited Loading

rkeithhill commented Mar 22, 2019 • edited Loading

rkeithhill commented Mar 22, 2019 • edited Loading

TylerLeonhardt commented Mar 22, 2019

rkeithhill commented Mar 22, 2019

bergmeister commented Mar 22, 2019

TylerLeonhardt commented Mar 22, 2019

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

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

TylerLeonhardt commented Mar 22, 2019 •

edited by rjmholt

Loading

rkeithhill commented Mar 22, 2019 •

edited

Loading

bergmeister commented Mar 22, 2019 •

edited

Loading

JamesWTruher commented Mar 22, 2019 •

edited

Loading

bergmeister commented Mar 22, 2019 •

edited

Loading

rkeithhill commented Mar 22, 2019 •

edited

Loading

rkeithhill commented Mar 22, 2019 •

edited

Loading