Reduce metadata contention while preserving runspace safety

Engine/CommandInfoCache.cs

@@ -25,6 +25,10 @@ internal class CommandInfoCache : IDisposable
         private const string GetCommandName = "Microsoft.PowerShell.Core\\Get-Command";
 
         private readonly ConcurrentDictionary<CommandLookupKey, Lazy<CommandInfo>> _commandInfoCache;
+        private readonly ConcurrentDictionary<CmdletInfo, IReadOnlyDictionary<string, CommandParameterSnapshot>> _parameterSnapshots
+            = new ConcurrentDictionary<CmdletInfo, IReadOnlyDictionary<string, CommandParameterSnapshot>>();
+        private readonly ConcurrentDictionary<CmdletInfo, IReadOnlyList<string>> _mandatoryParameters
+            = new ConcurrentDictionary<CmdletInfo, IReadOnlyList<string>>();
 
         /// <summary>
         /// Guards all access to <see cref="_runspace"/> so that only one thread at a time drives the
@@ -36,7 +40,7 @@ internal class CommandInfoCache : IDisposable
         private readonly object _runspaceLock = new object();
 
         private readonly Runspace _runspace;
-        private bool disposed = false;
+        private volatile bool disposed = false;
 
         /// <summary>
         /// Create a fresh command info cache instance.
@@ -62,7 +66,7 @@ protected virtual void Dispose(bool disposing)
             // Always take the lock, also on the finalizer path, so that 'disposed' is never
             // published without the runspace being disposed along with it and so that the runspace
             // cannot be disposed while a lookup is in flight.
-            lock (_runspaceLock)
+            using (PerformanceTelemetry.EnterLock(_runspaceLock))
             {
                 if ( disposed )
                 {
@@ -95,10 +99,14 @@ public CommandInfo GetCommandInfo(string commandName, CommandTypes? commandTypes
             var key = new CommandLookupKey(commandName, commandTypes);
             if (bypassCache)
             {
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.LookupBypasses);
                 return GetCommandInfoInternal(commandName, commandTypes);
             }
             // Atomically either use PowerShell to query a command info object, or fetch it from the cache
-            var lazyCommandInfo = _commandInfoCache.GetOrAdd(key, new Lazy<CommandInfo>(() => GetCommandInfoInternal(commandName, commandTypes)));
+            if (!_commandInfoCache.TryGetValue(key, out var lazyCommandInfo))
+            {
+                lazyCommandInfo = _commandInfoCache.GetOrAdd(key, CreateLookup(commandName, commandTypes));
+            }
             try
             {
                 return lazyCommandInfo.Value;
@@ -115,6 +123,14 @@ public CommandInfo GetCommandInfo(string commandName, CommandTypes? commandTypes
             }
         }
 
+        private Lazy<CommandInfo> CreateLookup(string commandName, CommandTypes? commandTypes)
+        {
+            return new Lazy<CommandInfo>(() =>
+            {
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.LookupMisses);
+                return GetCommandInfoInternal(commandName, commandTypes);
+            });
+        }
 
         /// <summary>
         /// Get a CommandInfo object of the given command name
@@ -145,7 +161,7 @@ private CommandInfo GetCommandInfoInternal(string cmdName, CommandTypes? command
             {
                 // Serialize all use of the PowerShell engine. Only cache misses reach this point;
                 // lookups that are already cached are served without taking the lock.
-                lock (_runspaceLock)
+                using (PerformanceTelemetry.EnterLock(_runspaceLock))
                 {
                     if (disposed)
                     {
@@ -219,11 +235,13 @@ public Dictionary<string, ParameterMetadata> GetCommandParameters(
             // Resolve the Lazy value before taking the lock: its factory may already be
             // running on another thread that needs the same lock to finish the lookup.
             var commandInfo = GetCommandInfo(commandName, commandTypes, bypassCache);
-            lock (_runspaceLock)
+            using (PerformanceTelemetry.EnterLock(_runspaceLock))
             {
                 // Dynamic parameter getters execute PowerShell code and mutate runspace state,
                 // even though they look like ordinary property reads.
-                return disposed ? null : commandInfo?.Parameters;
+                if (disposed || commandInfo == null) return null;
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.MetadataQueries);
+                return commandInfo.Parameters;
             }
         }
 
@@ -233,9 +251,71 @@ public Dictionary<string, ParameterMetadata> GetCommandParameters(
         public ReadOnlyCollection<CommandParameterSetInfo> GetCommandParameterSets(string commandName)
         {
             var commandInfo = GetCommandInfo(commandName);
-            lock (_runspaceLock)
+            using (PerformanceTelemetry.EnterLock(_runspaceLock))
+            {
+                if (disposed || commandInfo == null) return null;
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.MetadataQueries);
+                return commandInfo.ParameterSets;
+            }
+        }
+
+        private static CmdletInfo GetStaticCmdlet(CommandInfo command)
+        {
+            // Aliases, functions, subclasses and provider/dynamic cmdlets retain the locked,
+            // uncached path. IDynamicParameters includes implementations inherited from base types.
+            if (command == null || command.GetType() != typeof(CmdletInfo)) return null;
+            var cmdlet = (CmdletInfo)command;
+            return cmdlet.ImplementingType != null
+                && !typeof(IDynamicParameters).IsAssignableFrom(cmdlet.ImplementingType) ? cmdlet : null;
+        }
+
+        public IReadOnlyDictionary<string, CommandParameterSnapshot> GetParameterSnapshot(
+            string commandName, CommandTypes? commandTypes = null, bool bypassCache = false)
+        {
+            var command = GetCommandInfo(commandName, commandTypes, bypassCache);
+            var staticCmdlet = bypassCache ? null : GetStaticCmdlet(command);
+            if (disposed || command == null) return null;
+            if (staticCmdlet != null && _parameterSnapshots.TryGetValue(staticCmdlet, out var cached)) return cached;
+
+            using (PerformanceTelemetry.EnterLock(_runspaceLock))
             {
-                return disposed ? null : commandInfo?.ParameterSets;
+                if (disposed) return null;
+                if (staticCmdlet != null && _parameterSnapshots.TryGetValue(staticCmdlet, out cached)) return cached;
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.MetadataQueries);
+                var parameters = command.Parameters;
+                if (parameters == null) return null;
+                var snapshot = new ReadOnlyDictionary<string, CommandParameterSnapshot>(
+                    parameters.ToDictionary(p => p.Key, p => new CommandParameterSnapshot(p.Value), parameters.Comparer));
+                if (staticCmdlet != null) _parameterSnapshots[staticCmdlet] = snapshot;
+                return snapshot;
+            }
+        }
+
+        public IReadOnlyList<string> GetMandatoryParameterNames(string commandName)
+        {
+            var command = GetCommandInfo(commandName);
+            var staticCmdlet = GetStaticCmdlet(command);
+            if (disposed || command == null) return null;
+            if (staticCmdlet != null && _mandatoryParameters.TryGetValue(staticCmdlet, out var cached)) return cached;
+
+            using (PerformanceTelemetry.EnterLock(_runspaceLock))
+            {
+                if (disposed) return null;
+                if (staticCmdlet != null && _mandatoryParameters.TryGetValue(staticCmdlet, out cached)) return cached;
+                PerformanceTelemetry.Increment(ref PerformanceTelemetry.MetadataQueries);
+                int setCount = command.ParameterSets.Count;
+                var mandatory = new List<string>();
+                foreach (var parameter in command.Parameters.Values)
+                {
+                    if (parameter.Attributes.Count >= setCount
+                        && parameter.Attributes.OfType<ParameterAttribute>().Count(a => a.Mandatory) >= setCount)
+                    {
+                        mandatory.Add(parameter.Name);
+                    }
+                }
+                var snapshot = mandatory.AsReadOnly();
+                if (staticCmdlet != null) _mandatoryParameters[staticCmdlet] = snapshot;
+                return snapshot;
             }
         }
 
@@ -278,7 +358,7 @@ public override int GetHashCode()
                 unchecked
                 {
                     int hash = 17;
-                    hash = hash * 31 + Name.ToUpperInvariant().GetHashCode();
+                    hash = hash * 31 + StringComparer.OrdinalIgnoreCase.GetHashCode(Name);
                     hash = hash * 31 + CommandTypes.GetHashCode();
                     return hash;
                 }

Engine/CommandParameterSnapshot.cs

@@ -0,0 +1,24 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System.Collections.ObjectModel;
+using System.Linq;
+using System.Management.Automation;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
+{
+    /// <summary>Detached parameter facts, safe to consume without driving a PowerShell runspace.</summary>
+    public sealed class CommandParameterSnapshot
+    {
+        public string Name { get; }
+        public bool SwitchParameter { get; }
+        public ReadOnlyCollection<string> Aliases { get; }
+
+        internal CommandParameterSnapshot(ParameterMetadata parameter)
+        {
+            Name = parameter.Name;
+            SwitchParameter = parameter.SwitchParameter;
+            Aliases = new ReadOnlyCollection<string>(parameter.Aliases.ToArray());
+        }
+    }
+}

Engine/Helper.cs

@@ -294,7 +294,7 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable<ErrorReco
             PSModuleInfo psModuleInfo = null;
             Collection<PSObject> psObj = null;
             // Test-ModuleManifest is not thread safe
-            lock (_testModuleManifestLock)
+            using (PerformanceTelemetry.EnterLock(_testModuleManifestLock))
             {
                 using (var ps = System.Management.Automation.PowerShell.Create())
                 {
@@ -303,6 +303,7 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable<ErrorReco
                       .AddParameter("WarningAction", ActionPreference.SilentlyContinue);
                     try
                     {
+                        PerformanceTelemetry.Increment(ref PerformanceTelemetry.ManifestValidations);
                         psObj = ps.Invoke();
                     }
                     catch (CmdletInvocationException e)
@@ -330,6 +331,15 @@ public PSModuleInfo GetModuleManifest(string filePath, out IEnumerable<ErrorReco
             return psModuleInfo;
         }
 
+        /// <summary>Shares validation between built-in rules during one syntax-tree analysis only.</summary>
+        public PSModuleInfo GetModuleManifestForAnalysis(string filePath, out IEnumerable<ErrorRecord> errorRecords)
+        {
+            var cache = ModuleManifestAnalysisCache.Current;
+            return cache == null
+                ? GetModuleManifest(filePath, out errorRecords)
+                : cache.Get(this, filePath, out errorRecords);
+        }
+
         /// <summary>
         /// Checks if the error record is MissingMemberException
         /// </summary>
@@ -399,18 +409,22 @@ public HashSet<string> GetExportedFunction(Ast ast)
             List<string> exportFunctionsCmdlet = Helper.Instance.CmdletNameAndAliases("export-modulemember");
 
             // find functions exported
-            IEnumerable<Ast> cmdAsts = ast.FindAll(item => item is CommandAst
-                && exportFunctionsCmdlet.Contains((item as CommandAst).GetCommandName(), StringComparer.OrdinalIgnoreCase), true);
+            var cmdAsts = ast.FindAll(item => item is CommandAst
+                && exportFunctionsCmdlet.Contains((item as CommandAst).GetCommandName(), StringComparer.OrdinalIgnoreCase), true).ToArray();
+            if (cmdAsts.Length == 0)
+            {
+                return exportedFunctions;
+            }
 
             // Export-ModuleMember has no dynamic parameters. Resolve names from its static
             // metadata instead of ResolveParameter(), which re-enters the cached command's
             // runspace and races with command lookups and metadata queries on other rule threads.
-            var parameters = GetCommandParameters("export-modulemember", CommandTypes.Cmdlet);
+            var parameters = GetCommandParameterSnapshot("export-modulemember", CommandTypes.Cmdlet);
             if (parameters == null)
             {
                 return exportedFunctions;
             }
-            IEnumerable<ParameterMetadata> switchParams = parameters.Values.Where(pm => pm.SwitchParameter);
+            var switchParams = parameters.Values.Where(pm => pm.SwitchParameter);
 
             foreach (CommandAst cmdAst in cmdAsts)
             {
@@ -429,7 +443,7 @@ public HashSet<string> GetExportedFunction(Ast ast)
                     if (ceAst is CommandParameterAst)
                     {
                         var paramAst = ceAst as CommandParameterAst;
-                        ParameterMetadata param;
+                        CommandParameterSnapshot param;
                         if (!parameters.TryGetValue(paramAst.ParameterName, out param))
                         {
                             param = parameters.Values.FirstOrDefault(pm =>
@@ -701,6 +715,19 @@ public ReadOnlyCollection<CommandParameterSetInfo> GetCommandParameterSets(strin
             return CommandInfoCache.GetCommandParameterSets(name);
         }
 
+        /// <summary>Gets detached parameter facts; only static cmdlet metadata is cached.</summary>
+        public IReadOnlyDictionary<string, CommandParameterSnapshot> GetCommandParameterSnapshot(
+            string name, CommandTypes? commandType = null, bool bypassCache = false)
+        {
+            return CommandInfoCache.GetParameterSnapshot(name, commandType, bypassCache);
+        }
+
+        /// <summary>Gets the mandatory parameter summary under a single runspace lock.</summary>
+        public IReadOnlyList<string> GetMandatoryParameterNames(string name)
+        {
+            return CommandInfoCache.GetMandatoryParameterNames(name);
+        }
+
         /// <summary>
         /// Returns the get, set and test targetresource dsc function
         /// </summary>

Engine/ModuleManifestAnalysisCache.cs

@@ -0,0 +1,55 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+using System;
+using System.Collections.Generic;
+using System.Management.Automation;
+
+namespace Microsoft.Windows.PowerShell.ScriptAnalyzer
+{
+    // Owned by one AnalyzeSyntaxTree call and explicitly installed on its rule workers.
+    // It cannot outlive that analysis or affect unrelated public GetModuleManifest callers.
+    internal sealed class ModuleManifestAnalysisCache
+    {
+        [ThreadStatic]
+        internal static ModuleManifestAnalysisCache Current;
+
+        private readonly Dictionary<string, Result> results = new Dictionary<string, Result>(StringComparer.Ordinal);
+
+        internal PSModuleInfo Get(Helper helper, string path, out IEnumerable<ErrorRecord> errors)
+        {
+            lock (results)
+            {
+                if (!results.TryGetValue(path, out var result))
+                {
+                    var module = helper.GetModuleManifest(path, out var moduleErrors);
+                    result = new Result { Module = module, Errors = moduleErrors };
+                    results.Add(path, result);
+                }
+                errors = result.Errors;
+                return result.Module;
+            }
+        }
+
+        internal Scope Enter() => new Scope(this);
+
+        internal struct Scope : IDisposable
+        {
+            private readonly ModuleManifestAnalysisCache previous;
+
+            internal Scope(ModuleManifestAnalysisCache cache)
+            {
+                previous = Current;
+                Current = cache;
+            }
+
+            public void Dispose() => Current = previous;
+        }
+
+        private sealed class Result
+        {
+            internal PSModuleInfo Module;
+            internal IEnumerable<ErrorRecord> Errors;
+        }
+    }
+}