From b00ac0b9067779bee4464b8c457bdb2115d00dc4 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister (MVP)" Date: Sat, 16 Mar 2019 14:22:23 +0000 Subject: [PATCH 1/7] Speed up cold runs of PSSA by 20% by using a RunspacePool when querying PowerShell for CommandInfo. There are only 4 rules that could call the CommandInfoCache, therefore sizing the pool to 5. The pool creation is synchronous and adds 15ms of initialization overhead, which we can neglect I think. --- Engine/CommandInfoCache.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index 20fd211aa..8b23a48d8 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -5,6 +5,7 @@ using System.Collections.Concurrent; using System.Management.Automation; using System.Linq; +using System.Management.Automation.Runspaces; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -14,7 +15,7 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer internal class CommandInfoCache { private readonly ConcurrentDictionary> _commandInfoCache; - + private readonly RunspacePool _runspacePool; private readonly Helper _helperInstance; /// @@ -24,6 +25,9 @@ public CommandInfoCache(Helper pssaHelperInstance) { _commandInfoCache = new ConcurrentDictionary>(); _helperInstance = pssaHelperInstance; + // There are only 4 rules that use the CommandInfo cache and each rule does not request more than one concurrent command info request + _runspacePool = RunspaceFactory.CreateRunspacePool(1, 5); + _runspacePool.Open(); } /// @@ -67,10 +71,12 @@ public CommandInfo GetCommandInfoLegacy(string commandOrAliasName, CommandTypes? /// Get a CommandInfo object of the given command name /// /// Returns null if command does not exists - private static CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType) + private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType) { using (var ps = System.Management.Automation.PowerShell.Create()) { + ps.RunspacePool = _runspacePool; + ps.AddCommand("Get-Command") .AddParameter("Name", cmdName) .AddParameter("ErrorAction", "SilentlyContinue"); From 587ac7b4031823912e34d458c87ed9ca71956a51 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister (MVP)" Date: Sat, 16 Mar 2019 14:45:08 +0000 Subject: [PATCH 2/7] Use runspace pool for rule helper for manifests as well --- Engine/CommandInfoCache.cs | 8 +++----- Engine/Helper.cs | 39 ++++++++++---------------------------- 2 files changed, 13 insertions(+), 34 deletions(-) diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs index 8b23a48d8..0ed3f4569 100644 --- a/Engine/CommandInfoCache.cs +++ b/Engine/CommandInfoCache.cs @@ -15,19 +15,17 @@ namespace Microsoft.Windows.PowerShell.ScriptAnalyzer internal class CommandInfoCache { private readonly ConcurrentDictionary> _commandInfoCache; - private readonly RunspacePool _runspacePool; private readonly Helper _helperInstance; + private readonly RunspacePool _runspacePool; /// /// Create a fresh command info cache instance. /// - public CommandInfoCache(Helper pssaHelperInstance) + public CommandInfoCache(Helper pssaHelperInstance, RunspacePool runspacePool) { _commandInfoCache = new ConcurrentDictionary>(); _helperInstance = pssaHelperInstance; - // There are only 4 rules that use the CommandInfo cache and each rule does not request more than one concurrent command info request - _runspacePool = RunspaceFactory.CreateRunspacePool(1, 5); - _runspacePool.Open(); + _runspacePool = runspacePool; } /// diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 24c5a12c2..efeb70169 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -11,6 +11,7 @@ using System.Linq; using System.Management.Automation; using System.Management.Automation.Language; +using System.Management.Automation.Runspaces; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -29,6 +30,7 @@ public class Helper private PSVersionTable psVersionTable; private readonly Lazy _commandInfoCacheLazy; + private readonly RunspacePool _runSpacePool; #endregion @@ -113,7 +115,10 @@ internal set /// private Helper() { - _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this)); + // There are 5 rules that use the CommandInfo cache and each rule does not request more than one concurrent command info request + _runSpacePool = RunspaceFactory.CreateRunspacePool(1, 6); + _runSpacePool.Open(); + _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this, runspacePool: _runSpacePool)); } /// @@ -299,11 +304,12 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable psObj = null; using (var ps = System.Management.Automation.PowerShell.Create()) { + ps.RunspacePool = _runSpacePool; + ps.AddCommand("Test-ModuleManifest"); + ps.AddParameter("Path", filePath); + ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); try { - ps.AddCommand("Test-ModuleManifest"); - ps.AddParameter("Path", filePath); - ps.AddParameter("WarningAction", ActionPreference.SilentlyContinue); psObj = ps.Invoke(); } catch (CmdletInvocationException e) @@ -653,31 +659,6 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanTwoPositiona return moreThanTwoPositional ? argumentsWithoutProcedingParameters > 2 : argumentsWithoutProcedingParameters > 0; } - - /// - /// Get a CommandInfo object of the given command name - /// - /// Returns null if command does not exists - private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType) - { - using (var ps = System.Management.Automation.PowerShell.Create()) - { - var psCommand = ps.AddCommand("Get-Command") - .AddParameter("Name", cmdName) - .AddParameter("ErrorAction", "SilentlyContinue"); - - if(commandType!=null) - { - psCommand.AddParameter("CommandType", commandType); - } - - var commandInfo = psCommand.Invoke() - .FirstOrDefault(); - - return commandInfo; - } - } - /// /// Legacy method, new callers should use instead. /// Given a command's name, checks whether it exists. It does not use the passed in CommandTypes parameter, which is a bug. From f8e611463004dbc96dc6a709b64e04a78bd36820 Mon Sep 17 00:00:00 2001 From: "Christoph Bergmeister (MVP)" Date: Sat, 16 Mar 2019 17:40:15 +0000 Subject: [PATCH 3/7] Speed up cold starts of PSSA by another 35% by parallelising the expensive command lookup in AvoidAlias, which is the most expensive rule. To accomodate the higher parallel demand, increase runspace pool. --- Engine/Helper.cs | 5 ++-- Rules/AvoidAlias.cs | 61 +++++++++++++++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index efeb70169..b74637591 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -115,8 +115,9 @@ internal set /// private Helper() { - // There are 5 rules that use the CommandInfo cache and each rule does not request more than one concurrent command info request - _runSpacePool = RunspaceFactory.CreateRunspacePool(1, 6); + // There are 5 rules that use the CommandInfo cache but one rule (AvoidAlias) makes parallel queries. + // Therefore 10 runspaces was a heuristic measure where no more speed improvement was seen. + _runSpacePool = RunspaceFactory.CreateRunspacePool(1, 10); _runSpacePool.Open(); _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this, runspacePool: _runSpacePool)); } diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index ad35ec4fe..92cf35683 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -11,6 +11,8 @@ using System.Globalization; using System.Linq; using System.Management.Automation; +using System.Threading.Tasks; +using System.Collections.Concurrent; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer.BuiltinRules { @@ -95,7 +97,8 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) // Finds all CommandAsts. IEnumerable foundAsts = ast.FindAll(testAst => testAst is CommandAst, true); - // Iterates all CommandAsts and check the command name. + // Iterates all CommandAsts and check the command name. Expensive operations are run in background tasks + var tasks = new List>(); foreach (CommandAst cmdAst in foundAsts) { // Check if the command ast should be ignored @@ -128,24 +131,50 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) suggestedCorrections: GetCorrectionExtent(cmdAst, cmdletNameIfCommandNameWasAlias)); } - var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; - if (!isNativeCommand) + // Checking for implicit 'Get-' aliasing is done in a background task as it can be quite expensive during a cold-start + tasks.Add(Task.Run(() => { - var commdNameWithGetPrefix = $"Get-{commandName}"; - var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); - if (cmdletNameIfCommandWasMissingGetPrefix != null) - { - yield return new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix), - GetCommandExtent(cmdAst), - GetName(), - DiagnosticSeverity.Warning, - fileName, - commandName, - suggestedCorrections: GetCorrectionExtent(cmdAst, commdNameWithGetPrefix)); - } + return CheckForImplicitGetAliasing(commandName, cmdAst, fileName); + })); + } + foreach(var task in tasks) + { + var diagnosticRecordResult = task.Result; + if (diagnosticRecordResult != null) + { + yield return task.Result; + } + } + } + + /// + /// If one omitts, 'Get-' for a command, PowerShell will pre-pend it if such a command exists, therefore relying on such implicit aliasing is not desirable. + /// + /// + /// + /// + /// + private DiagnosticRecord CheckForImplicitGetAliasing(string commandName, CommandAst commandAst, string fileName) + { + var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; + if (!isNativeCommand) + { + var commdNameWithGetPrefix = $"Get-{commandName}"; + var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); + if (cmdletNameIfCommandWasMissingGetPrefix != null) + { + var diagnosticRecord = new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix), + GetCommandExtent(commandAst), + GetName(), + DiagnosticSeverity.Warning, + fileName, + commandName, + suggestedCorrections: GetCorrectionExtent(commandAst, commdNameWithGetPrefix)); + return diagnosticRecord; } } + return null; } /// From 785cb3f0fa149891a9d41ba75b66f900db320fb2 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Mon, 15 Apr 2019 19:36:43 +0100 Subject: [PATCH 4/7] Apply suggestions from code review Co-Authored-By: bergmeister --- Rules/AvoidAlias.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 92cf35683..93f91b7a9 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -137,9 +137,9 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) return CheckForImplicitGetAliasing(commandName, cmdAst, fileName); })); } - foreach(var task in tasks) + foreach(Task task in tasks) { - var diagnosticRecordResult = task.Result; + DiagnosticRecord diagnosticRecordResult = task.Result; if (diagnosticRecordResult != null) { yield return task.Result; @@ -156,7 +156,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// private DiagnosticRecord CheckForImplicitGetAliasing(string commandName, CommandAst commandAst, string fileName) { - var isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; + bool isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; if (!isNativeCommand) { var commdNameWithGetPrefix = $"Get-{commandName}"; @@ -293,4 +293,4 @@ public string GetSourceName() return string.Format(CultureInfo.CurrentCulture, Strings.SourceName); } } -} \ No newline at end of file +} From 250e9410732dc9754ee713812a4a4a2d311a4230 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 15 Apr 2019 22:00:06 +0100 Subject: [PATCH 5/7] Reduce indentation level --- Rules/AvoidAlias.cs | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/Rules/AvoidAlias.cs b/Rules/AvoidAlias.cs index 93f91b7a9..8ed979edb 100644 --- a/Rules/AvoidAlias.cs +++ b/Rules/AvoidAlias.cs @@ -157,24 +157,26 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) private DiagnosticRecord CheckForImplicitGetAliasing(string commandName, CommandAst commandAst, string fileName) { bool isNativeCommand = Helper.Instance.GetCommandInfo(commandName, CommandTypes.Application | CommandTypes.ExternalScript) != null; - if (!isNativeCommand) + if (isNativeCommand) { - var commdNameWithGetPrefix = $"Get-{commandName}"; - var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); - if (cmdletNameIfCommandWasMissingGetPrefix != null) - { - var diagnosticRecord = new DiagnosticRecord( - string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix), - GetCommandExtent(commandAst), - GetName(), - DiagnosticSeverity.Warning, - fileName, - commandName, - suggestedCorrections: GetCorrectionExtent(commandAst, commdNameWithGetPrefix)); - return diagnosticRecord; - } + return null; + } + var commdNameWithGetPrefix = $"Get-{commandName}"; + var cmdletNameIfCommandWasMissingGetPrefix = Helper.Instance.GetCommandInfo($"Get-{commandName}"); + if (cmdletNameIfCommandWasMissingGetPrefix == null) + { + return null; } - return null; + + var diagnosticRecord = new DiagnosticRecord( + string.Format(CultureInfo.CurrentCulture, Strings.AvoidUsingCmdletAliasesMissingGetPrefixError, commandName, commdNameWithGetPrefix), + GetCommandExtent(commandAst), + GetName(), + DiagnosticSeverity.Warning, + fileName, + commandName, + suggestedCorrections: GetCorrectionExtent(commandAst, commdNameWithGetPrefix)); + return diagnosticRecord; } /// From 1528e99a92368d20d8bb736f934dfd788bff2d56 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 15 Apr 2019 22:01:57 +0100 Subject: [PATCH 6/7] use build pattern for less repeated code --- Engine/Helper.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index b74637591..8f9c72119 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -306,9 +306,9 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable Date: Tue, 16 Apr 2019 07:22:19 +0100 Subject: [PATCH 7/7] trigger ci