From b5620bdfa973c201eac55d7d4930329547a69b40 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 01:53:21 -0800 Subject: [PATCH 1/8] Update GetCommandInfo method and cache results --- Engine/Helper.cs | 58 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 4573d4cd4..a24c9af17 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -38,6 +38,7 @@ public class Helper private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; + private Dictionary commandInfoCache; #endregion @@ -146,6 +147,7 @@ public void Initialize() KeywordBlockDictionary = new Dictionary>>(StringComparer.OrdinalIgnoreCase); VariableAnalysisDictionary = new Dictionary(); ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); + commandInfoCache = new Dictionary(StringComparer.OrdinalIgnoreCase); IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); @@ -610,12 +612,16 @@ public bool HasSplattedVariable(CommandAst cmdAst) /// public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositional = false) { - if (cmdAst == null || cmdAst.GetCommandName() == null) + if (cmdAst == null) { return false; } - CommandInfo commandInfo = GetCommandInfo(GetCmdletNameFromAlias(cmdAst.GetCommandName())) ?? GetCommandInfo(cmdAst.GetCommandName()); + var commandInfo = GetCommandInfo(cmdAst.GetCommandName()); + if (commandInfo == null || (commandInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet)) + { + return false; + } IEnumerable switchParams = null; @@ -685,6 +691,23 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio } + /// + /// Get a CommandInfo object of the given command name + /// + /// Returns null if command does not exists + private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType = null) + { + using (var ps = System.Management.Automation.PowerShell.Create()) + { + var cmdInfo = ps.AddCommand("Get-Command") + .AddArgument(cmdName) + .AddParameter("ErrorAction", "SilentlyContinue") + .Invoke() + .FirstOrDefault(); + return cmdInfo; + } + } + /// /// Given a command's name, checks whether it exists /// @@ -693,19 +716,28 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio /// public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) { - CommandTypes defaultCmdType = CommandTypes.Alias - | CommandTypes.Cmdlet - | CommandTypes.ExternalScript - | CommandTypes.Filter - | CommandTypes.Function - | CommandTypes.Script - | CommandTypes.Workflow; -#if !PSV3 - defaultCmdType |= CommandTypes.Configuration; -#endif + if (string.IsNullOrWhiteSpace(name)) + { + return null; + } + + // check if it is an alias + string cmdletName = Helper.Instance.GetCmdletNameFromAlias(name); + if (string.IsNullOrWhiteSpace(cmdletName)) + { + cmdletName = name; + } + lock (getCommandLock) { - return this.invokeCommand.GetCommand(name, commandType ?? defaultCmdType); + if (commandInfoCache.ContainsKey(cmdletName)) + { + return commandInfoCache[cmdletName]; + } + + var commandInfo = GetCommandInfoInternal(cmdletName, commandType); + commandInfoCache.Add(cmdletName, commandInfo); + return commandInfo; } } From a5c12c0a9a6e5a20ee3fe773814fa346b1ab685f Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 01:57:11 -0800 Subject: [PATCH 2/8] Make UseCmdletCorreclty use updated getcommandinfo --- Rules/UseCmdletCorrectly.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/Rules/UseCmdletCorrectly.cs b/Rules/UseCmdletCorrectly.cs index 5090de28e..6146711dc 100644 --- a/Rules/UseCmdletCorrectly.cs +++ b/Rules/UseCmdletCorrectly.cs @@ -55,7 +55,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) if (cmdAst.GetCommandName() == null) continue; // Checks mandatory parameters. - if (!IsMandatoryParameterExisted(cmdAst)) + if (!MandatoryParameterExists(cmdAst)) { yield return new DiagnosticRecord(String.Format(CultureInfo.CurrentCulture, Strings.UseCmdletCorrectlyError, cmdAst.GetCommandName()), cmdAst.Extent, GetName(), DiagnosticSeverity.Warning, fileName); @@ -68,7 +68,7 @@ public IEnumerable AnalyzeScript(Ast ast, string fileName) /// /// /// - private bool IsMandatoryParameterExisted(CommandAst cmdAst) + private bool MandatoryParameterExists(CommandAst cmdAst) { CommandInfo cmdInfo = null; List mandParams = new List(); @@ -88,9 +88,7 @@ private bool IsMandatoryParameterExisted(CommandAst cmdAst) #region Compares parameter list and mandatory parameter list. - cmdInfo = Helper.Instance.GetCommandInfo(Helper.Instance.GetCmdletNameFromAlias(cmdAst.GetCommandName())) - ?? Helper.Instance.GetCommandInfo(cmdAst.GetCommandName()); - + cmdInfo = Helper.Instance.GetCommandInfo(cmdAst.GetCommandName()); if (cmdInfo == null || (cmdInfo.CommandType != System.Management.Automation.CommandTypes.Cmdlet)) { return true; @@ -109,7 +107,7 @@ private bool IsMandatoryParameterExisted(CommandAst cmdAst) // If cannot find any mandatory parameter, it's not necessary to do a further check for current cmdlet. try { - int noOfParamSets = cmdInfo.ParameterSets.Count; + int noOfParamSets = cmdInfo.ParameterSets.Count; foreach (ParameterMetadata pm in cmdInfo.Parameters.Values) { int count = 0; @@ -163,7 +161,7 @@ private bool IsMandatoryParameterExisted(CommandAst cmdAst) return returnValue; } - + /// /// GetName: Retrieves the name of this rule. /// From 9c6e03cf2b9abfce29c73ce2ad318dabd73da660 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 01:59:00 -0800 Subject: [PATCH 3/8] Make UseShouldProcessCorrectly use updated getcommandinfo --- Rules/UseShouldProcessCorrectly.cs | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/Rules/UseShouldProcessCorrectly.cs b/Rules/UseShouldProcessCorrectly.cs index e56cd49f4..c7cb64296 100644 --- a/Rules/UseShouldProcessCorrectly.cs +++ b/Rules/UseShouldProcessCorrectly.cs @@ -297,29 +297,6 @@ private bool DeclaresSupportsShouldProcess(FunctionDefinitionAst ast) return Helper.Instance.GetNamedArgumentAttributeValue(shouldProcessAttribute); } - /// - /// Get a CommandInfo object of the given command name - /// - /// Returns null if command does not exists - private CommandInfo GetCommandInfo(string cmdName) - { - try - { - using (var ps = System.Management.Automation.PowerShell.Create()) - { - var cmdInfo = ps.AddCommand("Get-Command") - .AddArgument(cmdName) - .Invoke() - .FirstOrDefault(); - return cmdInfo; - } - } - catch (System.Management.Automation.CommandNotFoundException) - { - return null; - } - } - /// /// Checks if the given command supports ShouldProcess /// @@ -331,7 +308,7 @@ private bool SupportsShouldProcess(string cmdName) return false; } - var cmdInfo = GetCommandInfo(cmdName); + var cmdInfo = Helper.Instance.GetCommandInfo(cmdName); if (cmdInfo == null) { return false; From 2fae803328bfaa1161a840cb46556aa7d44d22ae Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 02:18:39 -0800 Subject: [PATCH 4/8] Add runspace pool to improve getcommandinfo perf --- Engine/Commands/GetScriptAnalyzerRuleCommand.cs | 14 ++++++++++++++ Engine/Commands/InvokeScriptAnalyzerCommand.cs | 2 ++ Engine/Helper.cs | 12 ++++++++++++ 3 files changed, 28 insertions(+) diff --git a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs index a0a669b98..ce738c75b 100644 --- a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs +++ b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs @@ -129,6 +129,20 @@ protected override void ProcessRecord() } } + protected override void EndProcessing() + { + ScriptAnalyzer.Instance.CleanUp(); + Helper.Instance.CleanUp(); + base.EndProcessing(); + } + + protected override void StopProcessing() + { + ScriptAnalyzer.Instance.CleanUp(); + Helper.Instance.CleanUp(); + base.StopProcessing(); + } + #endregion } } diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index 90fc2e9e1..e1f2066ad 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -337,12 +337,14 @@ protected override void ProcessRecord() protected override void EndProcessing() { ScriptAnalyzer.Instance.CleanUp(); + Helper.Instance.CleanUp(); base.EndProcessing(); } protected override void StopProcessing() { ScriptAnalyzer.Instance.CleanUp(); + Helper.Instance.CleanUp(); base.StopProcessing(); } diff --git a/Engine/Helper.cs b/Engine/Helper.cs index a24c9af17..c3a41483b 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -39,6 +39,7 @@ public class Helper private Dictionary> ruleArguments; private PSVersionTable psVersionTable; private Dictionary commandInfoCache; + private RunspacePool runspacePool; #endregion @@ -148,6 +149,8 @@ public void Initialize() VariableAnalysisDictionary = new Dictionary(); ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); commandInfoCache = new Dictionary(StringComparer.OrdinalIgnoreCase); + runspacePool = RunspaceFactory.CreateRunspacePool(InitialSessionState.CreateDefault2()); + runspacePool.Open(); IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); @@ -166,6 +169,14 @@ public void Initialize() } } + /// + /// We are forced to use this to improve performace + /// + public void CleanUp() + { + runspacePool.Dispose(); + } + /// /// Returns all the rule arguments /// @@ -699,6 +710,7 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command { using (var ps = System.Management.Automation.PowerShell.Create()) { + ps.RunspacePool = runspacePool; var cmdInfo = ps.AddCommand("Get-Command") .AddArgument(cmdName) .AddParameter("ErrorAction", "SilentlyContinue") From 585cca910c35c54019d0f51e695c9e63a999168c Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 03:09:38 -0800 Subject: [PATCH 5/8] Use concurrent dictionary for commandinfo cache --- Engine/Helper.cs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index c3a41483b..d6d139589 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -21,6 +21,7 @@ using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; using System.Management.Automation.Runspaces; using System.Collections; +using System.Collections.Concurrent; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -38,7 +39,7 @@ public class Helper private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; - private Dictionary commandInfoCache; + private ConcurrentDictionary commandInfoCache; private RunspacePool runspacePool; #endregion @@ -148,7 +149,7 @@ public void Initialize() KeywordBlockDictionary = new Dictionary>>(StringComparer.OrdinalIgnoreCase); VariableAnalysisDictionary = new Dictionary(); ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); - commandInfoCache = new Dictionary(StringComparer.OrdinalIgnoreCase); + commandInfoCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); runspacePool = RunspaceFactory.CreateRunspacePool(InitialSessionState.CreateDefault2()); runspacePool.Open(); @@ -740,17 +741,14 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) cmdletName = name; } - lock (getCommandLock) + if (commandInfoCache.ContainsKey(cmdletName)) { - if (commandInfoCache.ContainsKey(cmdletName)) - { - return commandInfoCache[cmdletName]; - } - - var commandInfo = GetCommandInfoInternal(cmdletName, commandType); - commandInfoCache.Add(cmdletName, commandInfo); - return commandInfo; + return commandInfoCache[cmdletName]; } + + var commandInfo = GetCommandInfoInternal(cmdletName, commandType); + commandInfoCache.AddOrUpdate(cmdletName, (key) => commandInfo, (key, value) => commandInfo); + return commandInfo; } /// From 20d862ee6a3fb5e0d831c7ca9c58f33e1e28b37a Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 03:10:15 -0800 Subject: [PATCH 6/8] Set max runspaces to 3 for getcommandinfo runspacepool --- Engine/Helper.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index d6d139589..7347546f1 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -151,6 +151,9 @@ public void Initialize() ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); commandInfoCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); runspacePool = RunspaceFactory.CreateRunspacePool(InitialSessionState.CreateDefault2()); + + // After some experimentation, I found out that setting max runspaces more than 3 has marginal returns. + runspacePool.SetMaxRunspaces(3); runspacePool.Open(); IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); From b660cc3d7b16b38eadb7a145db9e9cafaae02b0f Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 04:26:40 -0800 Subject: [PATCH 7/8] Remove redundant checks from some helper methods --- Engine/Helper.cs | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 7347546f1..a6514a54e 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -611,12 +611,13 @@ public string VariableNameWithoutScope(VariablePath variablePath) /// public bool HasSplattedVariable(CommandAst cmdAst) { - if (cmdAst == null || cmdAst.CommandElements == null) - { - return false; - } - - return cmdAst.CommandElements.Any(cmdElem => cmdElem is VariableExpressionAst && (cmdElem as VariableExpressionAst).Splatted); + return cmdAst != null + && cmdAst.CommandElements != null + && cmdAst.CommandElements.Any(cmdElem => + { + var varExprAst = cmdElem as VariableExpressionAst; + return varExprAst != null && varExprAst.Splatted; + }); } /// @@ -663,29 +664,31 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanThreePositio foreach (CommandElementAst ceAst in cmdAst.CommandElements) { - if (ceAst is CommandParameterAst) + var cmdParamAst = ceAst as CommandParameterAst; + if (cmdParamAst != null) + { + // Skip if it's a switch parameter + if (switchParams != null && + switchParams.Any( + pm => String.Equals( + pm.Name, + cmdParamAst.ParameterName, StringComparison.OrdinalIgnoreCase))) { - // Skip if it's a switch parameter - if (switchParams != null && - switchParams.Any(pm => String.Equals(pm.Name, (ceAst as CommandParameterAst).ParameterName, StringComparison.OrdinalIgnoreCase))) - { - continue; - } - - - parameters += 1; + continue; + } - if ((ceAst as CommandParameterAst).Argument != null) - { - arguments += 1; - } + parameters += 1; - } - else + if (cmdParamAst.Argument != null) { arguments += 1; } + } + else + { + arguments += 1; + } } // if not the first element in a pipeline, increase the number of arguments by 1 From 8726c10074d27fe466e64b675e59b9299efe6e41 Mon Sep 17 00:00:00 2001 From: Kapil Borle Date: Wed, 18 Jan 2017 20:09:30 -0800 Subject: [PATCH 8/8] Revert adding runspace pool and concurrent dictionary --- .../Commands/GetScriptAnalyzerRuleCommand.cs | 14 -------- .../Commands/InvokeScriptAnalyzerCommand.cs | 2 -- Engine/Helper.cs | 35 ++++++------------- 3 files changed, 11 insertions(+), 40 deletions(-) diff --git a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs index ce738c75b..a0a669b98 100644 --- a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs +++ b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs @@ -129,20 +129,6 @@ protected override void ProcessRecord() } } - protected override void EndProcessing() - { - ScriptAnalyzer.Instance.CleanUp(); - Helper.Instance.CleanUp(); - base.EndProcessing(); - } - - protected override void StopProcessing() - { - ScriptAnalyzer.Instance.CleanUp(); - Helper.Instance.CleanUp(); - base.StopProcessing(); - } - #endregion } } diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index e1f2066ad..90fc2e9e1 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -337,14 +337,12 @@ protected override void ProcessRecord() protected override void EndProcessing() { ScriptAnalyzer.Instance.CleanUp(); - Helper.Instance.CleanUp(); base.EndProcessing(); } protected override void StopProcessing() { ScriptAnalyzer.Instance.CleanUp(); - Helper.Instance.CleanUp(); base.StopProcessing(); } diff --git a/Engine/Helper.cs b/Engine/Helper.cs index a6514a54e..28e4d49a0 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -21,7 +21,6 @@ using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; using System.Management.Automation.Runspaces; using System.Collections; -using System.Collections.Concurrent; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -39,8 +38,7 @@ public class Helper private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; - private ConcurrentDictionary commandInfoCache; - private RunspacePool runspacePool; + private Dictionary commandInfoCache; #endregion @@ -149,12 +147,7 @@ public void Initialize() KeywordBlockDictionary = new Dictionary>>(StringComparer.OrdinalIgnoreCase); VariableAnalysisDictionary = new Dictionary(); ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); - commandInfoCache = new ConcurrentDictionary(StringComparer.OrdinalIgnoreCase); - runspacePool = RunspaceFactory.CreateRunspacePool(InitialSessionState.CreateDefault2()); - - // After some experimentation, I found out that setting max runspaces more than 3 has marginal returns. - runspacePool.SetMaxRunspaces(3); - runspacePool.Open(); + commandInfoCache = new Dictionary(StringComparer.OrdinalIgnoreCase); IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); @@ -173,14 +166,6 @@ public void Initialize() } } - /// - /// We are forced to use this to improve performace - /// - public void CleanUp() - { - runspacePool.Dispose(); - } - /// /// Returns all the rule arguments /// @@ -717,7 +702,6 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command { using (var ps = System.Management.Automation.PowerShell.Create()) { - ps.RunspacePool = runspacePool; var cmdInfo = ps.AddCommand("Get-Command") .AddArgument(cmdName) .AddParameter("ErrorAction", "SilentlyContinue") @@ -747,14 +731,17 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) cmdletName = name; } - if (commandInfoCache.ContainsKey(cmdletName)) + lock (getCommandLock) { - return commandInfoCache[cmdletName]; - } + if (commandInfoCache.ContainsKey(cmdletName)) + { + return commandInfoCache[cmdletName]; + } - var commandInfo = GetCommandInfoInternal(cmdletName, commandType); - commandInfoCache.AddOrUpdate(cmdletName, (key) => commandInfo, (key, value) => commandInfo); - return commandInfo; + var commandInfo = GetCommandInfoInternal(cmdletName, commandType); + commandInfoCache.Add(cmdletName, commandInfo); + return commandInfo; + } } ///