From cc664bc6390aec27b8b86a756f50fb4789a1273f Mon Sep 17 00:00:00 2001 From: Jason Shirk Date: Mon, 6 Feb 2017 23:09:55 -0800 Subject: [PATCH] Fix lock contention during script compilation (#3064) In rare cases, it was possible to cause high contention on a lock used while compiling code to run in the interpreter. This fixes the problem in a couple different ways: * Use ConditionalWeakTable which has finer granularity in locking * Avoid dictionary lookups in the most common cases - There really aren't too many real cases, I could have covered them all, but keeping the code generic is useful for the future. Not quite related, but I noticed we didn't cache certain interpreter instructions and it made sense to add a cache because we generate a lot of array-init instructions. --- .../engine/interpreter/InstructionFactory.cs | 71 ++++++++++--------- .../engine/interpreter/InstructionList.cs | 23 +++++- 2 files changed, 60 insertions(+), 34 deletions(-) diff --git a/src/System.Management.Automation/engine/interpreter/InstructionFactory.cs b/src/System.Management.Automation/engine/interpreter/InstructionFactory.cs index df68146b5..1edaa055f 100644 --- a/src/System.Management.Automation/engine/interpreter/InstructionFactory.cs +++ b/src/System.Management.Automation/engine/interpreter/InstructionFactory.cs @@ -13,6 +13,8 @@ * * ***************************************************************************/ + +using System.Collections.Concurrent; #if !CLR2 using BigInt = System.Numerics.BigInteger; #endif @@ -22,7 +24,8 @@ using BigInt = System.Numerics.BigInteger; using System.Reflection; #endif -using System.Collections.Generic; +using System.Runtime.CompilerServices; +using System.Threading; //using Microsoft.Scripting.Math; @@ -30,45 +33,34 @@ namespace System.Management.Automation.Interpreter { internal abstract class InstructionFactory { - // TODO: weak table for types in a collectible assembly? - private static Dictionary s_factories; + private static ConditionalWeakTable s_factories; internal static InstructionFactory GetFactory(Type type) { if (s_factories == null) { - s_factories = new Dictionary() { - { typeof(object), InstructionFactory.Factory }, - { typeof(bool), InstructionFactory.Factory }, - { typeof(byte), InstructionFactory.Factory }, - { typeof(sbyte), InstructionFactory.Factory }, - { typeof(short), InstructionFactory.Factory }, - { typeof(ushort), InstructionFactory.Factory }, - { typeof(int), InstructionFactory.Factory }, - { typeof(uint), InstructionFactory.Factory }, - { typeof(long), InstructionFactory.Factory }, - { typeof(ulong), InstructionFactory.Factory }, - { typeof(float), InstructionFactory.Factory }, - { typeof(double), InstructionFactory.Factory }, - { typeof(char), InstructionFactory.Factory }, - { typeof(string), InstructionFactory.Factory }, -#if !CLR2 - { typeof(BigInt), InstructionFactory.Factory }, -#endif - //{ typeof(BigInteger), InstructionFactory.Factory } - }; + var factories = new ConditionalWeakTable(); + factories.Add(typeof(object), InstructionFactory.Factory); + factories.Add(typeof(bool), InstructionFactory.Factory); + factories.Add(typeof(byte), InstructionFactory.Factory); + factories.Add(typeof(sbyte), InstructionFactory.Factory); + factories.Add(typeof(short), InstructionFactory.Factory); + factories.Add(typeof(ushort), InstructionFactory.Factory); + factories.Add(typeof(int), InstructionFactory.Factory); + factories.Add(typeof(uint), InstructionFactory.Factory); + factories.Add(typeof(long), InstructionFactory.Factory); + factories.Add(typeof(ulong), InstructionFactory.Factory); + factories.Add(typeof(float), InstructionFactory.Factory); + factories.Add(typeof(double), InstructionFactory.Factory); + factories.Add(typeof(char), InstructionFactory.Factory); + factories.Add(typeof(string), InstructionFactory.Factory); + factories.Add(typeof(BigInt), InstructionFactory.Factory); + + Interlocked.CompareExchange(ref s_factories, factories, null); } - lock (s_factories) - { - InstructionFactory factory; - if (!s_factories.TryGetValue(type, out factory)) - { - factory = (InstructionFactory)typeof(InstructionFactory<>).MakeGenericType(type).GetField("Factory").GetValue(null); - s_factories[type] = factory; - } - return factory; - } + return s_factories.GetValue(type, + t => (InstructionFactory)typeof(InstructionFactory<>).MakeGenericType(t).GetField("Factory").GetValue(null)); } protected internal abstract Instruction GetArrayItem(); @@ -91,6 +83,10 @@ namespace System.Management.Automation.Interpreter private Instruction _defaultValue; private Instruction _newArray; private Instruction _typeAs; + private Instruction[] _newArrayInit; + // This number is somewhat arbitrary - trying to avoid some gc without keeping + // objects (instructions) around that aren't used that often. + private const int MaxArrayInitElementCountCache = 32; private InstructionFactory() { } @@ -126,6 +122,15 @@ namespace System.Management.Automation.Interpreter protected internal override Instruction NewArrayInit(int elementCount) { + if (elementCount < MaxArrayInitElementCountCache) + { + if (_newArrayInit == null) + { + _newArrayInit = new Instruction[MaxArrayInitElementCountCache]; + } + + return _newArrayInit[elementCount] ?? (_newArrayInit[elementCount] = new NewArrayInitInstruction(elementCount)); + } return new NewArrayInitInstruction(elementCount); } } diff --git a/src/System.Management.Automation/engine/interpreter/InstructionList.cs b/src/System.Management.Automation/engine/interpreter/InstructionList.cs index bf55f7eb2..10260a1bf 100644 --- a/src/System.Management.Automation/engine/interpreter/InstructionList.cs +++ b/src/System.Management.Automation/engine/interpreter/InstructionList.cs @@ -749,7 +749,28 @@ namespace System.Management.Automation.Interpreter public void EmitNewArrayInit(Type elementType, int elementCount) { - Emit(InstructionFactory.GetFactory(elementType).NewArrayInit(elementCount)); + // To avoid lock contention in InstructionFactory.GetFactory, we special case the most common + // types of arrays that the compiler creates. + if (elementType == typeof(CommandParameterInternal)) + { + Emit(InstructionFactory.Factory.NewArrayInit(elementCount)); + } + else if (elementType == typeof(CommandParameterInternal[])) + { + Emit(InstructionFactory.Factory.NewArrayInit(elementCount)); + } + else if (elementType == typeof(object)) + { + Emit(InstructionFactory.Factory.NewArrayInit(elementCount)); + } + else if (elementType == typeof(string)) + { + Emit(InstructionFactory.Factory.NewArrayInit(elementCount)); + } + else + { + Emit(InstructionFactory.GetFactory(elementType).NewArrayInit(elementCount)); + } } #endregion