GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Formatter: Recycle parsed AST and tokens in between rule invocations when no correction was applied to improve performance by bergmeister · Pull Request #1462 · PowerShell/PSScriptAnalyzer

Conversation

@bergmeister

@bergmeister Christoph Bergmeister (bergmeister) commented Apr 26, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

PR Summary

This saves on the expensive calls to the Parser and the rule itself, which are called at the moment for every rule invocation AND after every applied DiagnosticRecord. With this PR, parsing is skipped and the previous result is used if a rule did not return a DiagnosticRecord that changed the analysed script definition string. If there is not DiagnosticRecord returned from a rule during formatting, this simple change can dramatically speed up the formatting time: In the case of a pre-formatted file (always using PowerShell's build.psm1 for reference), formatting is 3 times faster. In reality, I guess it's only around 50%-100% depending on how well formatter the script already is. Because I had to change publicly exposed methods, it is technically speaking a breaking change but I think we can live with that.

Since I haven't written this core logic, I can only guess it was written like this because applying a change invalidates the previous analysis and this explains why one cannot simply run the rules in parallel. However, I imagine, one could write code to not re-parse at all between applying a list of DiagnosticRecords by adapting the line and column numbers (which would also allow for parallelisation of the rules). At the moment one could probably even improve performance further by stopping rule analysis once the first object comes back due to this re-analysis pattern. I've actually tried that locally but it would've broken a few tests so maybe not as easy as it seems to be therefore let's leave that for later now though.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Christoph Bergmeister added 4 commits April 25, 2020 21:33
Christoph Bergmeister
Recycle Parse result for formatter, if a rule does not return a Diagn…
8cbd179 
…osticRecord and therefore does not require re-parsing
Christoph Bergmeister
fix issue where the parsing stage can only be skipped in the first lo…
5932cfa 
…op iteration of the Fix API
Christoph Bergmeister
Adapt LibraryUsage.tests.ps1
12a6f55
Christoph Bergmeister
Remove unnecessary call to ToList() and Count() where object is alrea…
e6ae0ff 
…dy a list
@bergmeister Christoph Bergmeister (bergmeister) added Area - Formatter Breaking Change Area - Performance labels Apr 26, 2020
@bergmeister
Christoph Bergmeister (bergmeister) requested review from James Truher (JamesWTruher) and Rob Holt (rjmholt) April 26, 2020 08:40
bergmeister
Christoph Bergmeister (bergmeister) commented Apr 26, 2020
View reviewed changes
Comment thread Engine/ScriptAnalyzer.cs

private static IEnumerable<CorrectionExtent> GetCorrectionExtentsForFix(
IEnumerable<CorrectionExtent> correctionExtents)
List<CorrectionExtent> correctionExtents)

@bergmeister Christoph Bergmeister (bergmeister) Apr 26, 2020

Copy link
Copy Markdown
Collaborator Author

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.

If one goes up in the call tree of the Fix methods then one sees that the passed through correctionExtents object is already of type List here, therefore this allows us to drop calls to .ToList() and .Count() here.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
rjmholt reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
bergmeister
Christoph Bergmeister (bergmeister) commented Apr 26, 2020
View reviewed changes
Comment thread Engine/ScriptAnalyzer.cs
/// <param name="skipVariableAnalysis">Whether variable analysis can be skipped (applicable if rules do not use variable analysis APIs).</param>
/// <returns></returns>
public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, bool skipVariableAnalysis = false)
public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, out ScriptBlockAst scriptAst, out Token[] scriptTokens, bool skipVariableAnalysis = false)

@bergmeister Christoph Bergmeister (bergmeister) Apr 26, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

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.

Technically speaking this is a breaking change but I don't think anyone depends on it (at least publicly on GitHub) and even if they did it should be easy for them to adopt. If it is of concerns, an alternative to not have a breaking change is to provide a wrapping layer like this

 public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefinition, bool skipVariableAnalysis = false)
{
    AnalyzeScriptDefinition(scriptDefinition, out _, out _, skipVariableAnalysis:skipVariableAnalysis )
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bergmeister
Christoph Bergmeister (bergmeister) commented Apr 26, 2020
View reviewed changes
Comment thread Engine/ScriptAnalyzer.cs
/// <param name="skipVariableAnalysis">Whether to skip variable analysis.</param>
/// <returns>The same instance of `EditableText` that was passed to the method, but the instance encapsulates the fixed script text. This helps in chaining the Fix method.</returns>
public EditableText Fix(EditableText text, Range range, out Range updatedRange, out bool fixesWereApplied, bool skipVariableAnalysis = false)
public EditableText Fix(EditableText text, Range range, bool skipParsing, out Range updatedRange, out bool fixesWereApplied, ref ScriptBlockAst scriptAst, ref Token[] scriptTokens, bool skipVariableAnalysis = false)

@bergmeister Christoph Bergmeister (bergmeister) Apr 26, 2020

Copy link
Copy Markdown
Collaborator Author

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.

Technically this is a breaking change as well but I think this class shouldn't have been public but rather internal in the first place. Again, we could still provide a wrapping layer for legacy purposed but it'd just leave the change as-is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@rjmholt Rob Holt (rjmholt) Apr 27, 2020

Copy link
Copy Markdown
Contributor

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.

I think this is acceptable; we've proceeded on the idea that the cmdlets are the real API so far and I think we can continue with that in the 1.x timeframe

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
rjmholt
Rob Holt (rjmholt) approved these changes Apr 27, 2020
View reviewed changes
Comment thread Engine/ScriptAnalyzer.cs
/// <param name="skipVariableAnalysis">Whether to skip variable analysis.</param>
/// <returns>The same instance of `EditableText` that was passed to the method, but the instance encapsulates the fixed script text. This helps in chaining the Fix method.</returns>
public EditableText Fix(EditableText text, Range range, out Range updatedRange, out bool fixesWereApplied, bool skipVariableAnalysis = false)
public EditableText Fix(EditableText text, Range range, bool skipParsing, out Range updatedRange, out bool fixesWereApplied, ref ScriptBlockAst scriptAst, ref Token[] scriptTokens, bool skipVariableAnalysis = false)

@rjmholt Rob Holt (rjmholt) Apr 27, 2020

Copy link
Copy Markdown
Contributor

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.

I think this is acceptable; we've proceeded on the idea that the cmdlets are the real API so far and I think we can continue with that in the 1.x timeframe

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Christoph Bergmeister
re-trigger ci
0d01812
@bergmeister
Christoph Bergmeister (bergmeister) merged commit 3e6987b into PowerShell:master Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JamesWTruher James Truher (JamesWTruher) Awaiting requested review from JamesWTruher
1 more reviewer
@rjmholt Rob Holt (rjmholt) rjmholt approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Area - Formatter Area - Performance Breaking Change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@bergmeister @rjmholt
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.