From aa6e147253e5a989cc352495870f0c466729ec53 Mon Sep 17 00:00:00 2001 From: Robert Holt Date: Wed, 13 Mar 2019 09:55:22 -0700 Subject: [PATCH] Increase lock granularity for CommandInfo cache (#1166) * Redo command info cache changes * Fix comment typo Co-Authored-By: rjmholt * Trigger CI --- Engine/CommandInfoCache.cs | 119 +++++++++++++++++++++++++++++++++++++ Engine/Helper.cs | 62 ++++--------------- 2 files changed, 131 insertions(+), 50 deletions(-) create mode 100644 Engine/CommandInfoCache.cs diff --git a/Engine/CommandInfoCache.cs b/Engine/CommandInfoCache.cs new file mode 100644 index 000000000..20fd211aa --- /dev/null +++ b/Engine/CommandInfoCache.cs @@ -0,0 +1,119 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using System.Collections.Concurrent; +using System.Management.Automation; +using System.Linq; + +namespace Microsoft.Windows.PowerShell.ScriptAnalyzer +{ + /// + /// Provides threadsafe caching around CommandInfo lookups with `Get-Command -Name ...`. + /// + internal class CommandInfoCache + { + private readonly ConcurrentDictionary> _commandInfoCache; + + private readonly Helper _helperInstance; + + /// + /// Create a fresh command info cache instance. + /// + public CommandInfoCache(Helper pssaHelperInstance) + { + _commandInfoCache = new ConcurrentDictionary>(); + _helperInstance = pssaHelperInstance; + } + + /// + /// Retrieve a command info object about a command. + /// + /// Name of the command to get a commandinfo object for. + /// The alias of the command to be used in the cache key. If not given, uses the command name. + /// What types of command are needed. If omitted, all types are retrieved. + /// + public CommandInfo GetCommandInfo(string commandName, string aliasName = null, CommandTypes? commandTypes = null) + { + if (string.IsNullOrWhiteSpace(commandName)) + { + return null; + } + + // If alias name is given, we store the entry under that, but search with the command name + var key = new CommandLookupKey(aliasName ?? commandName, commandTypes); + + // Atomically either use PowerShell to query a command info object, or fetch it from the cache + return _commandInfoCache.GetOrAdd(key, new Lazy(() => GetCommandInfoInternal(commandName, commandTypes))).Value; + } + + /// + /// Retrieve a command info object about a command. + /// + /// Name of the command to get a commandinfo object for. + /// What types of command are needed. If omitted, all types are retrieved. + /// + [Obsolete("Alias lookup is expensive and should not be relied upon for command lookup")] + public CommandInfo GetCommandInfoLegacy(string commandOrAliasName, CommandTypes? commandTypes = null) + { + string commandName = _helperInstance.GetCmdletNameFromAlias(commandOrAliasName); + + return string.IsNullOrEmpty(commandName) + ? GetCommandInfo(commandOrAliasName, commandTypes: commandTypes) + : GetCommandInfo(commandName, aliasName: commandOrAliasName, commandTypes: 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) + { + using (var ps = System.Management.Automation.PowerShell.Create()) + { + ps.AddCommand("Get-Command") + .AddParameter("Name", cmdName) + .AddParameter("ErrorAction", "SilentlyContinue"); + + if (commandType != null) + { + ps.AddParameter("CommandType", commandType); + } + + return ps.Invoke() + .FirstOrDefault(); + } + } + + private struct CommandLookupKey : IEquatable + { + private readonly string Name; + + private readonly CommandTypes CommandTypes; + + internal CommandLookupKey(string name, CommandTypes? commandTypes) + { + Name = name; + CommandTypes = commandTypes ?? CommandTypes.All; + } + + public bool Equals(CommandLookupKey other) + { + return CommandTypes == other.CommandTypes + && Name.Equals(other.Name, StringComparison.OrdinalIgnoreCase); + } + + public override int GetHashCode() + { + // Algorithm from https://stackoverflow.com/questions/1646807/quick-and-simple-hash-code-combinations + unchecked + { + int hash = 17; + hash = hash * 31 + Name.ToUpperInvariant().GetHashCode(); + hash = hash * 31 + CommandTypes.GetHashCode(); + return hash; + } + } + } + } +} diff --git a/Engine/Helper.cs b/Engine/Helper.cs index 03d9b3caa..24c5a12c2 100644 --- a/Engine/Helper.cs +++ b/Engine/Helper.cs @@ -24,11 +24,11 @@ public class Helper private CommandInvocationIntrinsics invokeCommand; private IOutputWriter outputWriter; - private Object getCommandLock = new object(); private readonly static Version minSupportedPSVersion = new Version(3, 0); private Dictionary> ruleArguments; private PSVersionTable psVersionTable; - private Dictionary commandInfoCache; + + private readonly Lazy _commandInfoCacheLazy; #endregion @@ -100,6 +100,12 @@ internal set private string[] functionScopes = new string[] { "global:", "local:", "script:", "private:"}; private string[] variableScopes = new string[] { "global:", "local:", "script:", "private:", "variable:", ":"}; + + /// + /// Store of command info objects for commands. Memoizes results. + /// + private CommandInfoCache CommandInfoCache => _commandInfoCacheLazy.Value; + #endregion /// @@ -107,7 +113,7 @@ internal set /// private Helper() { - + _commandInfoCacheLazy = new Lazy(() => new CommandInfoCache(pssaHelperInstance: this)); } /// @@ -123,7 +129,7 @@ private Helper() /// public Helper( CommandInvocationIntrinsics invokeCommand, - IOutputWriter outputWriter) + IOutputWriter outputWriter) : this() { this.invokeCommand = invokeCommand; this.outputWriter = outputWriter; @@ -140,10 +146,6 @@ public void Initialize() KeywordBlockDictionary = new Dictionary>>(StringComparer.OrdinalIgnoreCase); VariableAnalysisDictionary = new Dictionary(); ruleArguments = new Dictionary>(StringComparer.OrdinalIgnoreCase); - if (commandInfoCache == null) - { - commandInfoCache = new Dictionary(); - } IEnumerable aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true); @@ -677,7 +679,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. @@ -689,30 +690,7 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command [Obsolete] public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = null) { - if (string.IsNullOrWhiteSpace(name)) - { - return null; - } - - // check if it is an alias - string cmdletName = Helper.Instance.GetCmdletNameFromAlias(name); - if (string.IsNullOrWhiteSpace(cmdletName)) - { - cmdletName = name; - } - - var key = new CommandLookupKey(name, commandType); - lock (getCommandLock) - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - - var commandInfo = GetCommandInfoInternal(cmdletName, commandType); - commandInfoCache.Add(key, commandInfo); - return commandInfo; - } + return CommandInfoCache.GetCommandInfoLegacy(commandOrAliasName: name, commandTypes: commandType); } /// @@ -723,23 +701,7 @@ public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType = /// public CommandInfo GetCommandInfo(string name, CommandTypes? commandType = null) { - if (string.IsNullOrWhiteSpace(name)) - { - return null; - } - - var key = new CommandLookupKey(name, commandType); - lock (getCommandLock) - { - if (commandInfoCache.ContainsKey(key)) - { - return commandInfoCache[key]; - } - - var commandInfo = GetCommandInfoInternal(name, commandType); - commandInfoCache.Add(key, commandInfo); - return commandInfo; - } + return CommandInfoCache.GetCommandInfo(name, commandTypes: commandType); } ///