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.

Sign up for GitHub

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

Formatter: Recycle parsed AST and tokens in between rule invocations when no correction was applied to improve performance #1462

Merged
merged 5 commits into from
Apr 28, 2020
Merged

Formatter: Recycle parsed AST and tokens in between rule invocations when no correction was applied to improve performance #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8cbd179
Recycle Parse result for formatter, if a rule does not return a Diagn…
Apr 25, 2020
5932cfa
fix issue where the parsing stage can only be skipped in the first lo…
Apr 25, 2020
12a6f55
Adapt LibraryUsage.tests.ps1
Apr 25, 2020
e6ae0ff
Remove unnecessary call to ToList() and Count() where object is alrea…
Apr 26, 2020
0d01812
re-trigger ci
Apr 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Engine/Commands/InvokeScriptAnalyzerCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ private void ProcessInput()
}
else if (String.Equals(this.ParameterSetName, "ScriptDefinition", StringComparison.OrdinalIgnoreCase))
{
diagnosticsList = ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition);
diagnosticsList = ScriptAnalyzer.Instance.AnalyzeScriptDefinition(scriptDefinition, out _, out _);
WriteToOutput(diagnosticsList);
}
}
Expand Down
9 changes: 6 additions & 3 deletions Engine/Formatter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections;
using System.Management.Automation;
using System.Management.Automation.Language;

namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
{
Expand Down Expand Up @@ -46,6 +47,9 @@ public static string Format<TCmdlet>(
};

var text = new EditableText(scriptDefinition);
ScriptBlockAst scriptAst = null;
Token[] scriptTokens = null;
bool skipParsing = false;
foreach (var rule in ruleOrder)
{
if (!settings.RuleArguments.ContainsKey(rule))
Expand All @@ -57,9 +61,8 @@ public static string Format<TCmdlet>(
ScriptAnalyzer.Instance.UpdateSettings(currentSettings);
ScriptAnalyzer.Instance.Initialize(cmdlet, null, null, null, null, true, false);

Range updatedRange;
bool fixesWereApplied;
text = ScriptAnalyzer.Instance.Fix(text, range, out updatedRange, out fixesWereApplied, skipVariableAnalysis: true);
text = ScriptAnalyzer.Instance.Fix(text, range, skipParsing, out Range updatedRange, out bool fixesWereApplied, ref scriptAst, ref scriptTokens, skipVariableAnalysis: true);
skipParsing = !fixesWereApplied;
range = updatedRange;
}

Expand Down
51 changes: 32 additions & 19 deletions Engine/ScriptAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,8 +1478,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
if (shouldProcess(scriptFilePath, $"Analyzing and fixing file {scriptFilePath}"))
{
var fileEncoding = GetFileEncoding(scriptFilePath);
bool fixesWereApplied;
var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding), out fixesWereApplied);
var scriptFileContentWithFixes = Fix(File.ReadAllText(scriptFilePath, fileEncoding), out bool fixesWereApplied);
if (fixesWereApplied)
{
File.WriteAllText(scriptFilePath, scriptFileContentWithFixes, fileEncoding);
Expand All @@ -1497,12 +1496,15 @@ public IEnumerable<DiagnosticRecord> AnalyzeAndFixPath(string path, Func<string,
/// <summary>
/// Analyzes a script definition in the form of a string input
/// </summary>
/// <param name="scriptDefinition">The script to be analyzed</param>
/// <param name="scriptDefinition">The script to be analyzed.</param>
/// <param name="scriptAst">Parsed AST of <paramref name="scriptDefinition"/>.</param>
/// <param name="scriptTokens">Parsed tokens of <paramref name="scriptDefinition"/.></param>
/// <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)
Copy link
Collaborator Author

@bergmeister bergmeister Apr 26, 2020 •
edited
Loading

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

{
ScriptBlockAst scriptAst = null;
Token[] scriptTokens = null;
scriptAst = null;
scriptTokens = null;
ParseError[] errors = null;

this.outputWriter.WriteVerbose(string.Format(CultureInfo.CurrentCulture, Strings.VerboseScriptDefinitionMessage));
Expand Down Expand Up @@ -1546,7 +1548,7 @@ public IEnumerable<DiagnosticRecord> AnalyzeScriptDefinition(string scriptDefini
/// Fix the violations in the given script text.
/// </summary>
/// <param name="scriptDefinition">The script text to be fixed.</param>
/// <param name="updatedRange">Whether any warnings were fixed.</param>
/// <param name="fixesWereApplied">Whether any warnings were fixed.</param>
/// <returns>The fixed script text.</returns>
public string Fix(string scriptDefinition, out bool fixesWereApplied)
{
Expand All @@ -1555,20 +1557,24 @@ public string Fix(string scriptDefinition, out bool fixesWereApplied)
throw new ArgumentNullException(nameof(scriptDefinition));
}

Range updatedRange;
return Fix(new EditableText(scriptDefinition), null, out updatedRange, out fixesWereApplied).ToString();
ScriptBlockAst scriptAst = null;
Token[] scriptTokens = null;
return Fix(new EditableText(scriptDefinition), range: null, skipParsing: false, out _, out fixesWereApplied, ref scriptAst, ref scriptTokens).ToString();
}

/// <summary>
/// Fix the violations in the given script text.
/// </summary>
/// <param name="text">An object of type `EditableText` that encapsulates the script text to be fixed.</param>
/// <param name="text">An object of type <see cref="EditableText"/> that encapsulates the script text to be fixed.</param>
/// <param name="range">The range in which the fixes are allowed.</param>
/// <param name="skipParsing">Whether to use the <paramref name="scriptAst"/> and <paramref name="scriptTokens"/> parameters instead of parsing the <paramref name="text"/> parameter.</param>
/// <param name="updatedRange">The updated range after the fixes have been applied.</param>
/// <param name="updatedRange">Whether any warnings were fixed.</param>
/// <param name="fixesWereApplied">Whether any warnings were fixed.</param>
/// <param name="scriptAst">Optionally pre-parsed AST.</param>
/// <param name="scriptTokens">Optionally pre-parsed tokens.</param>
/// <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)
Copy link
Collaborator Author

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.

Copy link
Contributor

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

{
if (text == null)
{
Expand All @@ -1590,7 +1596,15 @@ public EditableText Fix(EditableText text, Range range, out Range updatedRange,
var previousUnusedCorrections = 0;
do
{
var records = AnalyzeScriptDefinition(text.ToString(), skipVariableAnalysis);
IEnumerable<DiagnosticRecord> records;
if (skipParsing && previousUnusedCorrections == 0)
{
records = AnalyzeSyntaxTree(scriptAst, scriptTokens, String.Empty, skipVariableAnalysis);
}
else
{
records = AnalyzeScriptDefinition(text.ToString(), out scriptAst, out scriptTokens, skipVariableAnalysis);
}
var corrections = records
.Select(r => r.SuggestedCorrections)
.Where(sc => sc != null && sc.Any())
Expand Down Expand Up @@ -1684,22 +1698,21 @@ private static Range SnapToEdges(EditableText text, Range range)
}

private static IEnumerable<CorrectionExtent> GetCorrectionExtentsForFix(
IEnumerable<CorrectionExtent> correctionExtents)
List<CorrectionExtent> correctionExtents)
Copy link
Collaborator Author

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.

{
var ceList = correctionExtents.ToList();
ceList.Sort((x, y) =>
correctionExtents.Sort((x, y) =>
{
return x.StartLineNumber < y.StartLineNumber ?
1 :
(x.StartLineNumber == y.StartLineNumber ? 0 : -1);
});

return ceList.GroupBy(ce => ce.StartLineNumber).Select(g => g.First());
return correctionExtents.GroupBy(ce => ce.StartLineNumber).Select(g => g.First());
}

private static EditableText Fix(
EditableText text,
IEnumerable<CorrectionExtent> correctionExtents,
List<CorrectionExtent> correctionExtents,
out int unusedCorrections)
{
var correctionsToUse = GetCorrectionExtentsForFix(correctionExtents);
Expand All @@ -1710,7 +1723,7 @@ private static EditableText Fix(
text.ApplyEdit(correction);
}

unusedCorrections = correctionExtents.Count() - count;
unusedCorrections = correctionExtents.Count - count;
return text;
}

Expand Down
2 changes: 1 addition & 1 deletion Tests/Engine/LibraryUsage.tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ function Invoke-ScriptAnalyzer {
}
else
{
$results = $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition);
$results = $scriptAnalyzer.AnalyzeScriptDefinition($ScriptDefinition, [ref] $null, [ref] $null)
}

$results
Expand Down