From d560c09b0296a0fa851f0f035d8ffeaeaa2c1e02 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 2 Mar 2019 21:36:23 +0000 Subject: [PATCH 1/7] Use ReaderWriterLockSlim for faster performance --- Engine/Helper.cs | 51 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index b093d9c0f..f08d97432 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.Threading; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -29,6 +30,7 @@ public class Helper private Dictionary> ruleArguments; private PSVersionTable psVersionTable; private Dictionary commandInfoCache; + private ReaderWriterLockSlim commandInfoCacheLock = new ReaderWriterLockSlim(); #endregion @@ -701,17 +703,33 @@ public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = } var key = new CommandLookupKey(name, commandType); - lock (getCommandLock) + commandInfoCacheLock.EnterReadLock(); + try + { + if (commandInfoCache.ContainsKey(key)) + { + return commandInfoCache[key]; + } + } + finally + { + commandInfoCacheLock.ExitReadLock(); + } + var commandInfo = GetCommandInfoInternal(cmdletName, commandType); + commandInfoCacheLock.EnterWriteLock(); + try { if (commandInfoCache.ContainsKey(key)) { return commandInfoCache[key]; } - - var commandInfo = GetCommandInfoInternal(cmdletName, commandType); commandInfoCache.Add(key, commandInfo); - return commandInfo; } + finally + { + commandInfoCacheLock.ExitWriteLock(); + } + return commandInfo; } /// @@ -728,17 +746,34 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) } var key = new CommandLookupKey(name, commandType); - lock (getCommandLock) + + commandInfoCacheLock.EnterReadLock(); + try + { + if (commandInfoCache.ContainsKey(key)) + { + return commandInfoCache[key]; + } + } + finally + { + commandInfoCacheLock.ExitReadLock(); + } + var commandInfo = GetCommandInfoInternal(name, commandType); + commandInfoCacheLock.EnterWriteLock(); + try { if (commandInfoCache.ContainsKey(key)) { return commandInfoCache[key]; } - - var commandInfo = GetCommandInfoInternal(name, commandType); commandInfoCache.Add(key, commandInfo); - return commandInfo; } + finally + { + commandInfoCacheLock.ExitWriteLock(); + } + return commandInfo; } /// From 5cf4249704c25bd753c21121a2bbc354c37f2387 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 2 Mar 2019 22:14:34 +0000 Subject: [PATCH 2/7] cleanup --- Engine/Helper.cs | 97 ++++++++++++++++++------------------------------ 1 file changed, 37 insertions(+), 60 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index f08d97432..7e044f5ed 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -24,8 +24,7 @@ public class Helper #region Private members private CommandInvocationIntrinsics invokeCommand; - private IOutputWriter outputWriter; - private Object getCommandLock = new object(); + private readonly IOutputWriter outputWriter; private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; @@ -654,9 +653,41 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanTwoPositiona /// - /// Get a CommandInfo object of the given command name + /// Get a CommandInfo object of the given command name. Uses the commandInfoCache for performance optimisation. /// /// Returns null if command does not exists + private CommandInfo GetCommandInfoInternalWithCommandCache(CommandLookupKey commandLookupKey, string cmdName, CommandTypes? commandType) + { + commandInfoCacheLock.EnterReadLock(); + try + { + if (commandInfoCache.ContainsKey(commandLookupKey)) + { + return commandInfoCache[commandLookupKey]; + } + } + finally + { + commandInfoCacheLock.ExitReadLock(); + } + var commandInfo = GetCommandInfoInternal(cmdName, commandType); + commandInfoCacheLock.EnterWriteLock(); + try + { + // Check Cache again because the command might have been added to it by another thread in the meantime + if (commandInfoCache.ContainsKey(commandLookupKey)) + { + return commandInfoCache[commandLookupKey]; + } + commandInfoCache.Add(commandLookupKey, commandInfo); + } + finally + { + commandInfoCacheLock.ExitWriteLock(); + } + return commandInfo; + } + private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType) { using (var ps = System.Management.Automation.PowerShell.Create()) @@ -665,7 +696,7 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command .AddParameter("Name", cmdName) .AddParameter("ErrorAction", "SilentlyContinue"); - if(commandType!=null) + if (commandType != null) { psCommand.AddParameter("CommandType", commandType); } @@ -678,7 +709,6 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command } /// - /// 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. /// But existing method callers are already depending on this behaviour and therefore this could not be simply fixed. @@ -703,33 +733,7 @@ public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = } var key = new CommandLookupKey(name, commandType); - commandInfoCacheLock.EnterReadLock(); - try - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - } - finally - { - commandInfoCacheLock.ExitReadLock(); - } - var commandInfo = GetCommandInfoInternal(cmdletName, commandType); - commandInfoCacheLock.EnterWriteLock(); - try - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - commandInfoCache.Add(key, commandInfo); - } - finally - { - commandInfoCacheLock.ExitWriteLock(); - } - return commandInfo; + return GetCommandInfoInternalWithCommandCache(key, cmdletName, commandType); } /// @@ -746,34 +750,7 @@ public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) } var key = new CommandLookupKey(name, commandType); - - commandInfoCacheLock.EnterReadLock(); - try - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - } - finally - { - commandInfoCacheLock.ExitReadLock(); - } - var commandInfo = GetCommandInfoInternal(name, commandType); - commandInfoCacheLock.EnterWriteLock(); - try - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - commandInfoCache.Add(key, commandInfo); - } - finally - { - commandInfoCacheLock.ExitWriteLock(); - } - return commandInfo; + return GetCommandInfoInternalWithCommandCache(key, name, commandType); } /// From 0e12540180d132e88c25e8e697275b2f4de8103f Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sat, 2 Mar 2019 22:25:26 +0000 Subject: [PATCH 3/7] re-order for cleaner diff and make lock readonly --- Engine/Helper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 7e044f5ed..346804eea 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -25,11 +25,11 @@ public class Helper private CommandInvocationIntrinsics invokeCommand; private readonly IOutputWriter outputWriter; + private readonly ReaderWriterLockSlim commandInfoCacheLock = new ReaderWriterLockSlim(); private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; private Dictionary commandInfoCache; - private ReaderWriterLockSlim commandInfoCacheLock = new ReaderWriterLockSlim(); #endregion From 6977c5682f31bd8214c470bb6c84dbb982f3ebb7 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 Mar 2019 21:09:53 +0000 Subject: [PATCH 4/7] remove redundant outputWriter and optimise using job management --- .../Commands/GetScriptAnalyzerRuleCommand.cs | 3 +- .../Commands/InvokeScriptAnalyzerCommand.cs | 3 +- Engine/Formatter.cs | 2 +- Engine/Helper.cs | 73 +++++++++++-------- Engine/ScriptAnalyzer.cs | 3 +- 5 files changed, 46 insertions(+), 38 deletions(-) diff --git a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs index 60ee1c607..bfe421e1f 100644 --- a/Engine/Commands/GetScriptAnalyzerRuleCommand.cs +++ b/Engine/Commands/GetScriptAnalyzerRuleCommand.cs @@ -84,8 +84,7 @@ protected override void BeginProcessing() // Initialize helper Helper.Instance = new Helper( - SessionState.InvokeCommand, - this); + SessionState.InvokeCommand); Helper.Instance.Initialize(); string[] rulePaths = Helper.ProcessCustomRulePaths(customRulePath, diff --git a/Engine/Commands/InvokeScriptAnalyzerCommand.cs b/Engine/Commands/InvokeScriptAnalyzerCommand.cs index c51fc9f38..7328eda5a 100644 --- a/Engine/Commands/InvokeScriptAnalyzerCommand.cs +++ b/Engine/Commands/InvokeScriptAnalyzerCommand.cs @@ -266,8 +266,7 @@ protected override void BeginProcessing() } #endif Helper.Instance = new Helper( - SessionState.InvokeCommand, - this); + SessionState.InvokeCommand); Helper.Instance.Initialize(); var psVersionTable = this.SessionState.PSVariable.GetValue("PSVersionTable") as Hashtable; diff --git a/Engine/Formatter.cs b/Engine/Formatter.cs index c07d7c5ef..c0845df07 100644 --- a/Engine/Formatter.cs +++ b/Engine/Formatter.cs @@ -31,7 +31,7 @@ public static string Format( ValidateNotNull(settings, "settings"); ValidateNotNull(cmdlet, "cmdlet"); - Helper.Instance = new Helper(cmdlet.SessionState.InvokeCommand, cmdlet); + Helper.Instance = new Helper(cmdlet.SessionState.InvokeCommand); Helper.Instance.Initialize(); var ruleOrder = new string[] diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 346804eea..508f6afba 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -4,6 +4,7 @@ using Microsoft.Windows.PowerShell.ScriptAnalyzer.Generic; using System; using System.Collections; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Globalization; @@ -12,6 +13,7 @@ using System.Management.Automation; using System.Management.Automation.Language; using System.Threading; +using System.Threading.Tasks; namespace Microsoft.Windows.PowerShell.ScriptAnalyzer { @@ -23,13 +25,13 @@ public class Helper { #region Private members - private CommandInvocationIntrinsics invokeCommand; - private readonly IOutputWriter outputWriter; + private readonly CommandInvocationIntrinsics invokeCommand; private readonly ReaderWriterLockSlim commandInfoCacheLock = new ReaderWriterLockSlim(); private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; - private Dictionary commandInfoCache; + private ConcurrentDictionary commandInfoCache; + private ConcurrentDictionary> commandInfoLookupItemsInProcess; #endregion @@ -122,12 +124,9 @@ private Helper() /// An IOutputWriter instance for use in writing output /// to the PowerShell environment. /// - public Helper( - CommandInvocationIntrinsics invokeCommand, - IOutputWriter outputWriter) + public Helper(CommandInvocationIntrinsics invokeCommand) { this.invokeCommand = invokeCommand; - this.outputWriter = outputWriter; } #region Methods @@ -143,7 +142,11 @@ public void Initialize() ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); if (commandInfoCache == null) { - commandInfoCache = new Dictionary(); + commandInfoCache = new ConcurrentDictionary(); + } + if (commandInfoLookupItemsInProcess == null) + { + commandInfoLookupItemsInProcess = new ConcurrentDictionary>(); } IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); @@ -653,43 +656,51 @@ public bool PositionalParameterUsed(CommandAst cmdAst, bool moreThanTwoPositiona /// - /// Get a CommandInfo object of the given command name. Uses the commandInfoCache for performance optimisation. + /// Get a CommandInfo object of the given command name using sophisticated caching. + /// Uses the commandInfoCache for performance optimisation. /// /// Returns null if command does not exists private CommandInfo GetCommandInfoInternalWithCommandCache(CommandLookupKey commandLookupKey, string cmdName, CommandTypes? commandType) { - commandInfoCacheLock.EnterReadLock(); - try + // Check if CommandInfo is cached + if (commandInfoCache.TryGetValue(commandLookupKey, out CommandInfo cachedCommandInfo)) { - if (commandInfoCache.ContainsKey(commandLookupKey)) - { - return commandInfoCache[commandLookupKey]; - } + return cachedCommandInfo; } - finally + + var commandInfoLookupTask = new Task(() => GetCommandInfoInternal(cmdName, commandType).Result); + if (commandInfoLookupItemsInProcess.TryAdd(commandLookupKey, commandInfoLookupTask)) { - commandInfoCacheLock.ExitReadLock(); + commandInfoLookupTask.Start(); + commandInfoLookupTask.Wait(); + commandInfoCache.TryAdd(commandLookupKey, commandInfoLookupTask.Result); + return commandInfoLookupTask.Result; } - var commandInfo = GetCommandInfoInternal(cmdName, commandType); - commandInfoCacheLock.EnterWriteLock(); - try + else { - // Check Cache again because the command might have been added to it by another thread in the meantime - if (commandInfoCache.ContainsKey(commandLookupKey)) + // Already processing a request for the same commandLookupKey + if (commandInfoLookupItemsInProcess.TryGetValue(commandLookupKey, out Task runningCommandInfoLookupTask)) { - return commandInfoCache[commandLookupKey]; + runningCommandInfoLookupTask.Wait(); + commandInfoCache.TryAdd(commandLookupKey, runningCommandInfoLookupTask.Result); + return runningCommandInfoLookupTask.Result; + } + else + { + // The CommandInfoLookup task in commandInfoLookupItemsInProcess has just finished now and must therefore be in the cache + if (commandInfoCache.TryGetValue(commandLookupKey, out CommandInfo commandInfo)) + { + return commandInfo; + } + // This case should never happen but is being catered for by just calling the function directly + return GetCommandInfoInternal(cmdName, commandType).Result; } - commandInfoCache.Add(commandLookupKey, commandInfo); - } - finally - { - commandInfoCacheLock.ExitWriteLock(); } - return commandInfo; } - private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType) + private Task GetCommandInfoInternal(string cmdName, CommandTypes? commandType) { + //Task.Factory.StartNew(() => using (var ps = System.Management.Automation.PowerShell.Create()) { var psCommand = ps.AddCommand("Get-Command") @@ -704,7 +715,7 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command var commandInfo = psCommand.Invoke() .FirstOrDefault(); - return commandInfo; + return Task.FromResult(commandInfo); } } diff --git a/Engine/ScriptAnalyzer.cs b/Engine/ScriptAnalyzer.cs index d7d091588..a3456b13b 100644 --- a/Engine/ScriptAnalyzer.cs +++ b/Engine/ScriptAnalyzer.cs @@ -159,8 +159,7 @@ public void Initialize( //initialize helper Helper.Instance = new Helper( - runspace.SessionStateProxy.InvokeCommand, - outputWriter); + runspace.SessionStateProxy.InvokeCommand); Helper.Instance.Initialize(); From e50361fca0e8734324a9a0289a0d4ceaf9c0d14e Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 Mar 2019 21:27:11 +0000 Subject: [PATCH 5/7] remove unused variable --- Engine/Helper.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 508f6afba..96258a845 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -26,7 +26,6 @@ public class Helper #region Private members private readonly CommandInvocationIntrinsics invokeCommand; - private readonly ReaderWriterLockSlim commandInfoCacheLock = new ReaderWriterLockSlim(); private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; From 66da684e6a7144c4bf57cf0507cd1c4888ffb5e9 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Sun, 3 Mar 2019 21:30:10 +0000 Subject: [PATCH 6/7] remove redundant comment --- Engine/Helper.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 96258a845..0d519706a 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -699,7 +699,6 @@ private CommandInfo GetCommandInfoInternalWithCommandCache(CommandLookupKey comm private Task GetCommandInfoInternal(string cmdName, CommandTypes? commandType) { - //Task.Factory.StartNew(() => using (var ps = System.Management.Automation.PowerShell.Create()) { var psCommand = ps.AddCommand("Get-Command") @@ -711,8 +710,7 @@ private Task GetCommandInfoInternal(string cmdName, CommandTypes? c psCommand.AddParameter("CommandType", commandType); } - var commandInfo = psCommand.Invoke() - .FirstOrDefault(); + var commandInfo = psCommand.Invoke().FirstOrDefault(); return Task.FromResult(commandInfo); } From 3555f3af7e7d4dda68abb55f6c71cd844efdf5b8 Mon Sep 17 00:00:00 2001 From: Christoph Bergmeister Date: Mon, 4 Mar 2019 07:30:07 +0000 Subject: [PATCH 7/7] remove items from processing list --- Engine/Helper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 0d519706a..e3f34e2ab 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -673,6 +673,7 @@ private CommandInfo GetCommandInfoInternalWithCommandCache(CommandLookupKey comm commandInfoLookupTask.Start(); commandInfoLookupTask.Wait(); commandInfoCache.TryAdd(commandLookupKey, commandInfoLookupTask.Result); + commandInfoLookupItemsInProcess.TryRemove(commandLookupKey, out _); return commandInfoLookupTask.Result; } else