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

Increase lock granularity for CommandInfo cache #1166

Merged
merged 3 commits into from
Mar 13, 2019
Merged

Increase lock granularity for CommandInfo cache #1166

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1f94f5
Redo command info cache changes
rjmholt Mar 6, 2019
f4f627a
Fix comment typo
bergmeister Mar 13, 2019
c2fab46
Trigger CI
bergmeister Mar 13, 2019
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
119 changes: 119 additions & 0 deletions Engine/CommandInfoCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
{
/// <summary>
/// Provides threadsafe caching around CommandInfo lookups with `Get-Command -Name ...`.
/// </summary>
internal class CommandInfoCache
{
private readonly ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>> _commandInfoCache;

private readonly Helper _helperInstance;

/// <summary>
/// Create a fresh command info cache instance.
/// </summary>
public CommandInfoCache(Helper pssaHelperInstance)
{
_commandInfoCache = new ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>>();
_helperInstance = pssaHelperInstance;
}

/// <summary>
/// Retrieve a command info object about a command.
/// </summary>
/// <param name="commandName">Name of the command to get a commandinfo object for.</param>
/// <param name="aliasName">The alias of the command to be used in the cache key. If not given, uses the command name.</param>
/// <param name="commandTypes">What types of command are needed. If omitted, all types are retrieved.</param>
/// <returns></returns>
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 catch
rjmholt marked this conversation as resolved.
Show resolved Hide resolved
return _commandInfoCache.GetOrAdd(key, new Lazy<CommandInfo>(() => GetCommandInfoInternal(commandName, commandTypes))).Value;
}

/// <summary>
/// Retrieve a command info object about a command.
/// </summary>
/// <param name="commandName">Name of the command to get a commandinfo object for.</param>
/// <param name="commandTypes">What types of command are needed. If omitted, all types are retrieved.</param>
/// <returns></returns>
[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);
}

/// <summary>
/// Get a CommandInfo object of the given command name
/// </summary>
/// <returns>Returns null if command does not exists</returns>
private static CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType)
{
using (var ps = System.Management.Automation.PowerShell.Create())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a chance to further improve the perf by using a static RunspacePool, instead of creating/disposing a Runspace for every call to this method.
Of course, you will need to decide the size of the pool carefully based on the typical scenarios.

The code would be something like this:

staitc RunspacePool rps = RunspaceFactory.CreateRunspacePool(minRunspaces: 1, maxRunspaces: 5)
...
private static CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? commandType)
{
    using (var ps = System.Management.Automation.PowerShell.Create())
    {
        ps.RunspacePool = rps;
        ps.AddCommand("Get-Command").Invoke()
        ...
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have tried something like this already in the past but could not get any significant measurable differences in terms of time. I think because all rules are run in separate threads, the rule that takes the longest is the weakest link in the chain and therefore the bottleneck.

Copy link
Member

@daxian-dbw daxian-dbw Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a RunspacePool here is mainly for two purpose:

  1. Make this method run faster
    • Runspace.Open is expensive.
    • module auto-loading and the cache populated during command discovery can be reused
    • RunspacePool may cause some extent of synchronization among the threads (more threads than the number of Runspaces get in this method). But only one invocation will be done by each thread, so the introduced synchronization wouldn't be that bad.
  2. Reduce the GC pressure
    • Open and dispose a Runspace in every thread that gets in this method would produce a lot shot living objects.

As for how effective it is to the overall run time improvement, maybe not much -- measurement speaks the truth :) But it looks to me a low hanging fruit.

{
ps.AddCommand("Get-Command")
.AddParameter("Name", cmdName)
.AddParameter("ErrorAction", "SilentlyContinue");

if(commandType != null)
{
ps.AddParameter("CommandType", commandType);
}

return ps.Invoke<CommandInfo>()
.FirstOrDefault();
}
}

private struct CommandLookupKey : IEquatable<CommandLookupKey>
{
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;
}
}
}
}
}
62 changes: 12 additions & 50 deletions Engine/Helper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, Dictionary<string, object>> ruleArguments;
private PSVersionTable psVersionTable;
private Dictionary<CommandLookupKey, CommandInfo> commandInfoCache;

private readonly Lazy<CommandInfoCache> _commandInfoCacheLazy;

#endregion

Expand Down Expand Up @@ -100,14 +100,20 @@ internal set
private string[] functionScopes = new string[] { "global:", "local:", "script:", "private:"};

private string[] variableScopes = new string[] { "global:", "local:", "script:", "private:", "variable:", ":"};

/// <summary>
/// Store of command info objects for commands. Memoizes results.
/// </summary>
private CommandInfoCache CommandInfoCache => _commandInfoCacheLazy.Value;

#endregion

/// <summary>
/// Initializes the Helper class.
/// </summary>
private Helper()
{

_commandInfoCacheLazy = new Lazy<CommandInfoCache>(() => new CommandInfoCache(pssaHelperInstance: this));
}

/// <summary>
Expand All @@ -123,7 +129,7 @@ private Helper()
/// </param>
public Helper(
CommandInvocationIntrinsics invokeCommand,
IOutputWriter outputWriter)
IOutputWriter outputWriter) : this()
{
this.invokeCommand = invokeCommand;
this.outputWriter = outputWriter;
Expand All @@ -140,10 +146,6 @@ public void Initialize()
KeywordBlockDictionary = new Dictionary<String, List<Tuple<int, int>>>(StringComparer.OrdinalIgnoreCase);
VariableAnalysisDictionary = new Dictionary<Ast, VariableAnalysis>();
ruleArguments = new Dictionary<string, Dictionary<string, object>>(StringComparer.OrdinalIgnoreCase);
if (commandInfoCache == null)
{
commandInfoCache = new Dictionary<CommandLookupKey, CommandInfo>();
}

IEnumerable<CommandInfo> aliases = this.invokeCommand.GetCommands("*", CommandTypes.Alias, true);

Expand Down Expand Up @@ -676,7 +678,6 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command
}

/// <summary>

/// Legacy method, new callers should use <see cref="GetCommandInfo"/> 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.
Expand All @@ -688,30 +689,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);
Copy link
Collaborator

@bergmeister bergmeister Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look carefully, the name variable is being used for the creation of the key but the cmdletname variable is passed into this function. The name and cmdletname variable might not always be the same but you pass only name into the new GetCommandInfo() function. I think you'd have to pass the CommandLookupKey as well into it instead to preserve legacy behaviour. We have seen it very hard to make any change to this method without breaking something else, hence why this method is marked as legacy. Unless you can argue that they are always the same, I'd be careful changing it (even if tests are green)

commandInfoCache.Add(key, commandInfo);
return commandInfo;
}
return CommandInfoCache.GetCommandInfoLegacy(commandOrAliasName: name, commandTypes: commandType);
}

/// <summary>
Expand All @@ -722,23 +700,7 @@ public CommandInfo GetCommandInfoLegacy(string name, CommandTypes? commandType =
/// <returns></returns>
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);
}

/// <summary>
Expand Down