Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upIncrease lock granularity for CommandInfo cache #1166
Conversation
| return commandInfoCache[key]; | ||
| } | ||
|
|
||
| var commandInfo = GetCommandInfoInternal(cmdletName, commandType); |
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 5, 2019
•
Collaborator
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)
|
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 |
| /// <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()) |
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Mar 8, 2019
Member
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()
...
}
}
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 12, 2019
Collaborator
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.
This comment has been minimized.
This comment has been minimized.
daxian-dbw
Mar 13, 2019
•
Member
Using a RunspacePool here is mainly for two purpose:
- Make this method run faster
Runspace.Openis expensive.- module auto-loading and the cache populated during command discovery can be reused
RunspacePoolmay cause some extent of synchronization among the threads (more threads than the number ofRunspacesget in this method). But only one invocation will be done by each thread, so the introduced synchronization wouldn't be that bad.
- Reduce the GC pressure
- Open and dispose a
Runspacein every thread that gets in this method would produce a lot shot living objects.
- Open and dispose a
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.
|
Thanks, looks good to me. I am happy to close my PR #1162 in favour of this |
* Redo command info cache changes * Fix comment typo Co-Authored-By: rjmholt <rjmholt@gmail.com> * Trigger CI
rjmholt commentedMar 5, 2019
•
edited by bergmeister
PR Summary
Introduces a concurrent dictionary allowing different caller threads to get
CommandInfoobjects for different commands without blocking each other.Alternative implementation of #1162.
Closes #1162
More details:
ConcurrentDictionary<K, V>.GetOrAdd()), so very little opportunity to introduce problems laterPR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.