From 7272b1fad8ead98acbff8ca8febb9cb220cce83a Mon Sep 17 00:00:00 2001 From: iSazonov Date: Thu, 2 Aug 2018 18:54:17 +0500 Subject: [PATCH 01/10] Remove extra allocations in PSMemberInfoInternalCollection PSMemberInfoInternalCollection uses internally OrderedDictionary class. The OrderedDictionary class in .Net Core 2.1 allocates internally ArrayList and Hashtable after any first using any property. Even if we check that OrderedDictionary is empty (Count==0) or use indexer it causes allocations. The commit postpones the internall allocations until this is really necessary. Taking into account that PSMemberInfoInternalCollection is used in each PSObject and is often empty, this change dramatically reduces memory allocations. --- .../engine/MshMemberInfo.cs | 80 +++++++++++++------ 1 file changed, 55 insertions(+), 25 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index 35c6acecedd..306ad855dcb 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4008,20 +4008,32 @@ public virtual IEnumerator GetEnumerator() /// internal class PSMemberInfoInternalCollection : PSMemberInfoCollection, IEnumerable where T : PSMemberInfo { - private readonly OrderedDictionary _members; + private OrderedDictionary _members; private int _countHidden; + private OrderedDictionary Members + { + get + { + if (_members == null) + { + _members = new OrderedDictionary(StringComparer.OrdinalIgnoreCase); + } + + return _members; + } + } + /// /// Constructs this collection /// internal PSMemberInfoInternalCollection() { - _members = new OrderedDictionary(StringComparer.OrdinalIgnoreCase); } private void Replace(T oldMember, T newMember) { - _members[newMember.Name] = newMember; + Members[newMember.Name] = newMember; if (oldMember.IsHidden) { _countHidden--; @@ -4040,9 +4052,9 @@ internal void Replace(T newMember) { Diagnostics.Assert(newMember != null, "called from internal code that checks for new member not null"); - lock (_members) + lock (Members) { - var oldMember = _members[newMember.Name] as T; + var oldMember = Members[newMember.Name] as T; Diagnostics.Assert(oldMember != null, "internal code checks member already exists"); Replace(oldMember, newMember); } @@ -4075,16 +4087,15 @@ public override void Add(T member, bool preValidated) throw PSTraceSource.NewArgumentNullException("member"); } - lock (_members) + lock (Members) { - var existingMember = _members[member.Name] as T; - if (existingMember != null) + if (Members[member.Name] is T existingMember) { Replace(existingMember, member); } else { - _members[member.Name] = member; + Members[member.Name] = member; if (member.IsHidden) { _countHidden++; @@ -4114,16 +4125,15 @@ public override void Remove(string name) name); } - lock (_members) + lock (Members) { - var member = _members[name] as PSMemberInfo; - if (member != null) + if (Members[name] is PSMemberInfo member) { if (member.IsHidden) { _countHidden--; } - _members.Remove(name); + Members.Remove(name); } } } @@ -4143,9 +4153,14 @@ public override T this[string name] throw PSTraceSource.NewArgumentException("name"); } - lock (_members) + if (_members == null) + { + return null; + } + + lock (Members) { - return _members[name] as T; + return Members[name] as T; } } } @@ -4203,9 +4218,9 @@ internal override ReadOnlyPSMemberInfoCollection Match(string name, PSMemberT private PSMemberInfoInternalCollection GetInternalMembers(MshMemberMatchOptions matchOptions) { PSMemberInfoInternalCollection returnValue = new PSMemberInfoInternalCollection(); - lock (_members) + lock (Members) { - foreach (T member in _members.Values.OfType()) + foreach (T member in Members.Values.OfType()) { if (member.MatchesOptions(matchOptions)) { @@ -4224,9 +4239,14 @@ internal int Count { get { - lock (_members) + if (_members == null) + { + return 0; + } + + lock (Members) { - return _members.Count; + return Members.Count; } } } @@ -4238,9 +4258,14 @@ internal int VisibleCount { get { - lock (_members) + if (_members == null) + { + return 0; + } + + lock (Members) { - return _members.Count - _countHidden; + return Members.Count - _countHidden; } } } @@ -4254,9 +4279,14 @@ internal T this[int index] { get { - lock (_members) + if (_members == null) + { + return null; + } + + lock (Members) { - return _members[index] as T; + return Members[index] as T; } } } @@ -4269,10 +4299,10 @@ internal T this[int index] /// the enumerator for this collection public override IEnumerator GetEnumerator() { - lock (_members) + lock (Members) { // Copy the members to a list so that iteration can be performed without holding a lock. - return _members.Values.OfType().ToList().GetEnumerator(); + return Members.Values.OfType().ToList().GetEnumerator(); } } } From 4d900c6a45f32dca2babaec2fff11a89a7f9683d Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:08:55 +0500 Subject: [PATCH 02/10] Use _member in lock --- .../engine/MshMemberInfo.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index 306ad855dcb..07ce045a5ca 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4052,7 +4052,7 @@ internal void Replace(T newMember) { Diagnostics.Assert(newMember != null, "called from internal code that checks for new member not null"); - lock (Members) + lock (_members) { var oldMember = Members[newMember.Name] as T; Diagnostics.Assert(oldMember != null, "internal code checks member already exists"); @@ -4087,7 +4087,7 @@ public override void Add(T member, bool preValidated) throw PSTraceSource.NewArgumentNullException("member"); } - lock (Members) + lock (_members) { if (Members[member.Name] is T existingMember) { @@ -4125,7 +4125,7 @@ public override void Remove(string name) name); } - lock (Members) + lock (_members) { if (Members[name] is PSMemberInfo member) { @@ -4158,7 +4158,7 @@ public override T this[string name] return null; } - lock (Members) + lock (_members) { return Members[name] as T; } @@ -4218,7 +4218,7 @@ internal override ReadOnlyPSMemberInfoCollection Match(string name, PSMemberT private PSMemberInfoInternalCollection GetInternalMembers(MshMemberMatchOptions matchOptions) { PSMemberInfoInternalCollection returnValue = new PSMemberInfoInternalCollection(); - lock (Members) + lock (_members) { foreach (T member in Members.Values.OfType()) { @@ -4244,7 +4244,7 @@ internal int Count return 0; } - lock (Members) + lock (_members) { return Members.Count; } @@ -4263,7 +4263,7 @@ internal int VisibleCount return 0; } - lock (Members) + lock (_members) { return Members.Count - _countHidden; } @@ -4284,7 +4284,7 @@ internal T this[int index] return null; } - lock (Members) + lock (_members) { return Members[index] as T; } @@ -4299,7 +4299,7 @@ internal T this[int index] /// the enumerator for this collection public override IEnumerator GetEnumerator() { - lock (Members) + lock (_members) { // Copy the members to a list so that iteration can be performed without holding a lock. return Members.Values.OfType().ToList().GetEnumerator(); From 15900b8e791110b273eb9c0109dd43929d46b4c2 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:35:52 +0500 Subject: [PATCH 03/10] Return empty enumerator --- src/System.Management.Automation/engine/MshMemberInfo.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index 07ce045a5ca..60810663e48 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4299,6 +4299,11 @@ internal T this[int index] /// the enumerator for this collection public override IEnumerator GetEnumerator() { + if (_members == null) + { + return Enumerable.Empty().GetEnumerator(); + } + lock (_members) { // Copy the members to a list so that iteration can be performed without holding a lock. From 8ccf586f5ee56483a7383c913afe08b647194ce6 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:39:29 +0500 Subject: [PATCH 04/10] Use Interlocked.CompareExchange in initialization code --- src/System.Management.Automation/engine/MshMemberInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index 60810663e48..e72b3ffc166 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4017,7 +4017,7 @@ private OrderedDictionary Members { if (_members == null) { - _members = new OrderedDictionary(StringComparer.OrdinalIgnoreCase); + System.Threading.Interlocked.CompareExchange(ref _members, new OrderedDictionary(StringComparer.OrdinalIgnoreCase), null); } return _members; From 6ad4a71379af09ef79a5d0818087ac9f8396952c Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:49:00 +0500 Subject: [PATCH 05/10] Add short return path in Remove() method --- src/System.Management.Automation/engine/MshMemberInfo.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index e72b3ffc166..fce7801a4c0 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4125,6 +4125,11 @@ public override void Remove(string name) name); } + if (_members == null) + { + return; + } + lock (_members) { if (Members[name] is PSMemberInfo member) From d94a03f3773cb14d9bbd08947a9674fb0689514c Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:51:02 +0500 Subject: [PATCH 06/10] Fix Count and VisiableCount --- src/System.Management.Automation/engine/MshMemberInfo.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index fce7801a4c0..c2c4227ec6a 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4251,7 +4251,7 @@ internal int Count lock (_members) { - return Members.Count; + return _members.Count; } } } @@ -4270,7 +4270,7 @@ internal int VisibleCount lock (_members) { - return Members.Count - _countHidden; + return _members.Count - _countHidden; } } } From feecfd600d92342e4956f8173cf50235987f1a49 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 08:52:16 +0500 Subject: [PATCH 07/10] Fix indexer --- src/System.Management.Automation/engine/MshMemberInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index c2c4227ec6a..f2ca818bf2e 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4291,7 +4291,7 @@ internal T this[int index] lock (_members) { - return Members[index] as T; + return _members[index] as T; } } } From fb9efec9a1a45d1d1c1e80898062db129ca252d7 Mon Sep 17 00:00:00 2001 From: iSazonov Date: Fri, 3 Aug 2018 16:40:55 +0500 Subject: [PATCH 08/10] Revert commit "Use _member in lock" --- .../engine/MshMemberInfo.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index f2ca818bf2e..c9bcfae2c89 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4052,7 +4052,7 @@ internal void Replace(T newMember) { Diagnostics.Assert(newMember != null, "called from internal code that checks for new member not null"); - lock (_members) + lock (Members) { var oldMember = Members[newMember.Name] as T; Diagnostics.Assert(oldMember != null, "internal code checks member already exists"); @@ -4087,7 +4087,7 @@ public override void Add(T member, bool preValidated) throw PSTraceSource.NewArgumentNullException("member"); } - lock (_members) + lock (Members) { if (Members[member.Name] is T existingMember) { @@ -4130,7 +4130,7 @@ public override void Remove(string name) return; } - lock (_members) + lock (Members) { if (Members[name] is PSMemberInfo member) { @@ -4163,7 +4163,7 @@ public override T this[string name] return null; } - lock (_members) + lock (Members) { return Members[name] as T; } @@ -4223,7 +4223,7 @@ internal override ReadOnlyPSMemberInfoCollection Match(string name, PSMemberT private PSMemberInfoInternalCollection GetInternalMembers(MshMemberMatchOptions matchOptions) { PSMemberInfoInternalCollection returnValue = new PSMemberInfoInternalCollection(); - lock (_members) + lock (Members) { foreach (T member in Members.Values.OfType()) { @@ -4249,7 +4249,7 @@ internal int Count return 0; } - lock (_members) + lock (Members) { return _members.Count; } @@ -4268,7 +4268,7 @@ internal int VisibleCount return 0; } - lock (_members) + lock (Members) { return _members.Count - _countHidden; } @@ -4289,7 +4289,7 @@ internal T this[int index] return null; } - lock (_members) + lock (Members) { return _members[index] as T; } @@ -4309,7 +4309,7 @@ public override IEnumerator GetEnumerator() return Enumerable.Empty().GetEnumerator(); } - lock (_members) + lock (Members) { // Copy the members to a list so that iteration can be performed without holding a lock. return Members.Values.OfType().ToList().GetEnumerator(); From 9bd2e0790d77f7560f3286c3a7e4308c623cadbc Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 3 Aug 2018 10:27:42 -0700 Subject: [PATCH 09/10] [Feature] Minor updates --- .../engine/MshMemberInfo.cs | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index c9bcfae2c89..ec136bb9838 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4011,6 +4011,10 @@ internal class PSMemberInfoInternalCollection : PSMemberInfoCollection, IE private OrderedDictionary _members; private int _countHidden; + /// + /// Get the OrderedDictionary for holding all members. + /// We use this property to delay initializing _members until we absolutely need to. + /// private OrderedDictionary Members { get @@ -4130,15 +4134,15 @@ public override void Remove(string name) return; } - lock (Members) + lock (_members) { - if (Members[name] is PSMemberInfo member) + if (_members[name] is PSMemberInfo member) { if (member.IsHidden) { _countHidden--; } - Members.Remove(name); + _members.Remove(name); } } } @@ -4163,9 +4167,9 @@ public override T this[string name] return null; } - lock (Members) + lock (_members) { - return Members[name] as T; + return _members[name] as T; } } } @@ -4223,9 +4227,15 @@ internal override ReadOnlyPSMemberInfoCollection Match(string name, PSMemberT private PSMemberInfoInternalCollection GetInternalMembers(MshMemberMatchOptions matchOptions) { PSMemberInfoInternalCollection returnValue = new PSMemberInfoInternalCollection(); - lock (Members) + + if (_members == null) { - foreach (T member in Members.Values.OfType()) + return returnValue; + } + + lock (_members) + { + foreach (T member in _members.Values.OfType()) { if (member.MatchesOptions(matchOptions)) { @@ -4249,7 +4259,7 @@ internal int Count return 0; } - lock (Members) + lock (_members) { return _members.Count; } @@ -4268,7 +4278,7 @@ internal int VisibleCount return 0; } - lock (Members) + lock (_members) { return _members.Count - _countHidden; } @@ -4289,7 +4299,7 @@ internal T this[int index] return null; } - lock (Members) + lock (_members) { return _members[index] as T; } @@ -4309,10 +4319,10 @@ public override IEnumerator GetEnumerator() return Enumerable.Empty().GetEnumerator(); } - lock (Members) + lock (_members) { // Copy the members to a list so that iteration can be performed without holding a lock. - return Members.Values.OfType().ToList().GetEnumerator(); + return _members.Values.OfType().ToList().GetEnumerator(); } } } From 57dd2c4d86b7aaaffd481a932b24ee7f49ebc4c9 Mon Sep 17 00:00:00 2001 From: Dongbo Wang Date: Fri, 3 Aug 2018 11:10:28 -0700 Subject: [PATCH 10/10] [Feature] Address comment --- .../engine/MshMemberInfo.cs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/MshMemberInfo.cs b/src/System.Management.Automation/engine/MshMemberInfo.cs index ec136bb9838..3b752599a6d 100644 --- a/src/System.Management.Automation/engine/MshMemberInfo.cs +++ b/src/System.Management.Automation/engine/MshMemberInfo.cs @@ -4012,7 +4012,7 @@ internal class PSMemberInfoInternalCollection : PSMemberInfoCollection, IE private int _countHidden; /// - /// Get the OrderedDictionary for holding all members. + /// Gets the OrderedDictionary for holding all members. /// We use this property to delay initializing _members until we absolutely need to. /// private OrderedDictionary Members @@ -4056,9 +4056,11 @@ internal void Replace(T newMember) { Diagnostics.Assert(newMember != null, "called from internal code that checks for new member not null"); - lock (Members) + // Save to a local variable to reduce property access. + var members = Members; + lock (members) { - var oldMember = Members[newMember.Name] as T; + var oldMember = members[newMember.Name] as T; Diagnostics.Assert(oldMember != null, "internal code checks member already exists"); Replace(oldMember, newMember); } @@ -4091,15 +4093,17 @@ public override void Add(T member, bool preValidated) throw PSTraceSource.NewArgumentNullException("member"); } - lock (Members) + // Save to a local variable to reduce property access. + var members = Members; + lock (members) { - if (Members[member.Name] is T existingMember) + if (members[member.Name] is T existingMember) { Replace(existingMember, member); } else { - Members[member.Name] = member; + members[member.Name] = member; if (member.IsHidden) { _countHidden++;