Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFormatter: Recycle parsed AST and tokens in between rule invocations when no correction was applied to improve performance #1462
Conversation
| @@ -1684,22 +1698,21 @@ private static Range SnapToEdges(EditableText text, Range range) | |||
| } | |||
|
|
|||
| private static IEnumerable<CorrectionExtent> GetCorrectionExtentsForFix( | |||
| IEnumerable<CorrectionExtent> correctionExtents) | |||
| List<CorrectionExtent> correctionExtents) | |||
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 26, 2020
Author
Collaborator
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.
| /// <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) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 26, 2020
•
Author
Collaborator
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 )
}| /// <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) |
This comment has been minimized.
This comment has been minimized.
bergmeister
Apr 26, 2020
Author
Collaborator
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.
This comment has been minimized.
This comment has been minimized.
rjmholt
Apr 27, 2020
Member
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
| /// <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) |
This comment has been minimized.
This comment has been minimized.
rjmholt
Apr 27, 2020
Member
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
3e6987b
into
PowerShell:master
bergmeister commentedApr 26, 2020
•
edited
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 aDiagnosticRecordthat changed the analysed script definition string. If there is notDiagnosticRecordreturned 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'sbuild.psm1for 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
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.