Skip to content

Remove extra allocations in PSMemberInfoInternalCollection<T> #7435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daxian-dbw merged 10 commits into from
Aug 6, 2018
Merged

Remove extra allocations in PSMemberInfoInternalCollection<T> #7435

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7272b1f
Remove extra allocations in PSMemberInfoInternalCollection<T>
iSazonov Aug 2, 2018
4d900c6
Use _member in lock
iSazonov Aug 3, 2018
15900b8
Return empty enumerator
iSazonov Aug 3, 2018
8ccf586
Use Interlocked.CompareExchange in initialization code
iSazonov Aug 3, 2018
6ad4a71
Add short return path in Remove() method
iSazonov Aug 3, 2018
d94a03f
Fix Count and VisiableCount
iSazonov Aug 3, 2018
feecfd6
Fix indexer
iSazonov Aug 3, 2018
fb9efec
Revert commit "Use _member in lock"
iSazonov Aug 3, 2018
9bd2e07
[Feature] Minor updates
daxian-dbw Aug 3, 2018
57dd2c4
[Feature] Address comment
daxian-dbw Aug 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 65 additions & 11 deletions src/System.Management.Automation/engine/MshMemberInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4008,20 +4008,36 @@ public virtual IEnumerator<T> GetEnumerator()
/// </summary>
internal class PSMemberInfoInternalCollection<T> : PSMemberInfoCollection<T>, IEnumerable<T> where T : PSMemberInfo
{
private readonly OrderedDictionary _members;
private OrderedDictionary _members;
private int _countHidden;

/// <summary>
/// Gets the OrderedDictionary for holding all members.
/// We use this property to delay initializing _members until we absolutely need to.
/// </summary>
private OrderedDictionary Members
{
get
{
if (_members == null)
{
System.Threading.Interlocked.CompareExchange(ref _members, new OrderedDictionary(StringComparer.OrdinalIgnoreCase), null);
}

return _members;
}
}

/// <summary>
/// Constructs this collection
/// </summary>
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--;
Expand All @@ -4040,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);
}
Expand Down Expand Up @@ -4075,16 +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)
{
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++;
Expand Down Expand Up @@ -4114,10 +4133,14 @@ public override void Remove(string name)
name);
}

if (_members == null)
{
return;
}

lock (_members)
{
var member = _members[name] as PSMemberInfo;
if (member != null)
if (_members[name] is PSMemberInfo member)
{
if (member.IsHidden)
{
Expand All @@ -4143,6 +4166,11 @@ public override T this[string name]
throw PSTraceSource.NewArgumentException("name");
}

if (_members == null)
{
return null;
}

lock (_members)
{
return _members[name] as T;
Expand Down Expand Up @@ -4203,6 +4231,12 @@ internal override ReadOnlyPSMemberInfoCollection<T> Match(string name, PSMemberT
private PSMemberInfoInternalCollection<T> GetInternalMembers(MshMemberMatchOptions matchOptions)
{
PSMemberInfoInternalCollection<T> returnValue = new PSMemberInfoInternalCollection<T>();

if (_members == null)
{
return returnValue;
}

lock (_members)
{
foreach (T member in _members.Values.OfType<T>())
Expand All @@ -4224,6 +4258,11 @@ internal int Count
{
get
{
if (_members == null)
{
return 0;
}

lock (_members)
{
return _members.Count;
Expand All @@ -4238,6 +4277,11 @@ internal int VisibleCount
{
get
{
if (_members == null)
{
return 0;
}

lock (_members)
{
return _members.Count - _countHidden;
Expand All @@ -4254,6 +4298,11 @@ internal T this[int index]
{
get
{
if (_members == null)
{
return null;
}

lock (_members)
{
return _members[index] as T;
Expand All @@ -4269,6 +4318,11 @@ internal T this[int index]
/// <returns>the enumerator for this collection</returns>
public override IEnumerator<T> GetEnumerator()
{
if (_members == null)
{
return Enumerable.Empty<T>().GetEnumerator();
}

lock (_members)
{
// Copy the members to a list so that iteration can be performed without holding a lock.
Expand Down