Increase lock granularity for CommandInfo cache

[rjmholt]

PR Summary

Introduces a concurrent dictionary allowing different caller threads to get CommandInfo objects for different commands without blocking each other.

Alternative implementation of #1162.
Closes #1162

More details:

• Spins the commandinfo cache into its own object, so that the complexity of thread management is managed in one place
• Uses a single concurrency primitive (ConcurrentDictionary<K, V>.GetOrAdd()), so very little opportunity to introduce problems later
• Does not use tasks
• Has the same performance characteristics as the task-based implementation

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Change is not breaking
• Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
• Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[bergmeister]

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)

[bergmeister]

Thanks for the efforts, I see now what you mean, yes it looks more elegant to me, I didn't know one could do caching with lambda expressions this way as well (that's why I thought originally I had to use Task). I'd have preferred it if the changeset was kept smaller. Looks ok-ish but I have 1 comment about trying to preserve legacy behaviour to reduce the risk of a potential regression.

[rjmholt]

Thanks for the efforts, I see now what you mean, yes it looks more elegant to me, I didn't know one could do caching with lambda expressions this way as well (that's why I thought originally I had to use Task). I'd have preferred it if the changeset was kept smaller. Looks ok-ish but I have 1 comment about trying to preserve legacy behaviour to reduce the risk of a potential regression.

Fair points, I'll undo the unnecessary changes

[daxian-dbw]

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()
        ...
    }
}

[bergmeister]

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.

[daxian-dbw]

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.

[bergmeister]

Thanks, looks good to me. I am happy to close my PR #1162 in favour of this