Skip to content
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.

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

Devirtualize TokenMap #47274

Merged
7 commits merged into from
Sep 5, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
94 changes: 55 additions & 39 deletions src/Compilers/Core/Portable/CodeGen/TokenMap.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
using System.Threading;
using Microsoft.Cci;

namespace Microsoft.CodeAnalysis.CodeGen
{
Expand All @@ -24,74 +23,91 @@ namespace Microsoft.CodeAnalysis.CodeGen
/// </summary>
internal sealed class TokenMap
{
private readonly ConcurrentDictionary<object, uint> _itemIdentityToToken = new ConcurrentDictionary<object, uint>(ReferenceEqualityComparer.Instance);
private readonly ConcurrentDictionary<IReferenceOrISignature, uint> _itemIdentityToToken = new();
private readonly Dictionary<IReferenceOrISignatureEquivalent, uint> _itemEquivalentToToken = new();
private object[] _items = Array.Empty<object>();
private int _count = 0;

private readonly Dictionary<object, uint> _itemToToken;
private readonly ArrayBuilder<object> _items = new ArrayBuilder<object>();
internal TokenMap()
{
}

internal TokenMap(IEqualityComparer<object> comparer)
public uint GetOrAddTokenFor(IReference item, out bool referenceAdded)
{
_itemToToken = new Dictionary<object, uint>(comparer);
if (_itemIdentityToToken.TryGetValue(new IReferenceOrISignature(item), out uint token))
{
referenceAdded = false;
return token;
}

return AddItem(new IReferenceOrISignatureEquivalent(item), out referenceAdded);
}

public uint GetOrAddTokenFor(object item, out bool referenceAdded)
public uint GetOrAddTokenFor(ISignature item, out bool referenceAdded)
{
uint tmp;
if (_itemIdentityToToken.TryGetValue(item, out tmp))
if (_itemIdentityToToken.TryGetValue(new IReferenceOrISignature(item), out uint token))
{
referenceAdded = false;
return (uint)tmp;
return token;
}

return AddItem(item, out referenceAdded);
return AddItem(new IReferenceOrISignatureEquivalent(item), out referenceAdded);
}

private uint AddItem(object item, out bool referenceAdded)
private uint AddItem(IReferenceOrISignatureEquivalent item, out bool referenceAdded)
{
Debug.Assert(item is Cci.ISignature || item is Cci.IReference);
uint token;

// NOTE: cannot use GetOrAdd here since items and itemToToken must be in sync
// so if we do need to add we have to take a lock and modify both collections.
lock (_items)
lock (_itemEquivalentToToken)
{
if (!_itemToToken.TryGetValue(item, out token))
// Check if there is an equivalent type that has a token
if (!_itemEquivalentToToken.TryGetValue(item, out token))
{
token = (uint)_items.Count;
_items.Add(item);
_itemToToken.Add(item, token);
token = (uint)_count;
// No equivalent, add the token for this type
_itemEquivalentToToken.Add(item, token);

var count = (int)token + 1;
var items = _items;
if (items.Length > count)
{
items[(int)token] = item.AsObject();
}
else
{
// Not enough room, we need to resize the array
Array.Resize(ref items, Math.Max(8, count * 2));
items[(int)token] = item.AsObject();

// Update the updated array reference before updating _count
Volatile.Write(ref _items, items);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this safe? It seems very likely to me that _items could have been changed by this point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only location _items is updated and it is done in a lock. The Volatile.Write is to ensure its not reordered with the write to _count; so it's safe to Volatile.Read them in reverse order in GetAllItems() since _count only ever increases

}

Volatile.Write(ref _count, count);
}
}

// Use the provided token to update the reference dictionary
referenceAdded = _itemIdentityToToken.TryAdd(item, token);
return token;
}

public object GetItem(uint token)
{
lock (_items)
{
return _items[(int)token];
}
}

public IEnumerable<object> GetAllItems()
{
lock (_items)
{
return _items.ToArray();
}
return _items[(int)token];
}

//TODO: why is this is called twice during emit?
// should probably return ROA instead of IE and cache that in Module. (and no need to return count)
public IEnumerable<object> GetAllItemsAndCount(out int count)
public object[] GetAllItems()
{
lock (_items)
{
count = _items.Count;
return _items.ToArray();
}
// Read the count prior to getting the array
int count = Volatile.Read(ref _count);
object[] items = Volatile.Read(ref _items);

// Return a right sized copy of the array
benaadams marked this conversation as resolved.
Show resolved Hide resolved
return (new ReadOnlySpan<object>(items, 0, count)).ToArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem particularly safe to me either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider that AddItem does volatile writes in the following order: _items then _count. This method now reads them volatiley in the other order _count then _items. That means by the time we read _count here we know there are at least _count elements in the _items collection hence the array returned here will be well formed (no null values).

There is a subtle invariant change from the previous version of the code:

  • Before: If this method was entered when AddItem was executing then it would return the effect of completing the AddItem method
  • After: If this method was entered when AddItem was executing then it may or may not return the effect of completing the AddItem method.

Don't believe we care about maintaining that invariant.

benaadams marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
10 changes: 5 additions & 5 deletions src/Compilers/Core/Portable/Emit/CommonPEModuleBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ internal abstract class CommonPEModuleBuilder : Cci.IUnit, Cci.IModuleReference
internal Cci.IMethodReference DebugEntryPoint;

private readonly ConcurrentDictionary<IMethodSymbolInternal, Cci.IMethodBody> _methodBodyMap;
private readonly TokenMap _referencesInILMap = new TokenMap(MetadataEntityReferenceComparer.ConsiderEverything);
private readonly ItemTokenMap<string> _stringsInILMap = new ItemTokenMap<string>();
private readonly ItemTokenMap<Cci.DebugSourceDocument> _sourceDocumentsInILMap = new ItemTokenMap<Cci.DebugSourceDocument>();
private readonly TokenMap _referencesInILMap = new();
private readonly ItemTokenMap<string> _stringsInILMap = new();
private readonly ItemTokenMap<Cci.DebugSourceDocument> _sourceDocumentsInILMap = new();

private ImmutableArray<Cci.AssemblyReferenceAlias> _lazyAssemblyReferenceAliases;
private ImmutableArray<Cci.ManagedResource> _lazyManagedResources;
Expand Down Expand Up @@ -335,9 +335,9 @@ public string GetStringFromToken(uint token)
return _stringsInILMap.GetItem(token);
}

public IEnumerable<object> ReferencesInIL(out int count)
public object[] ReferencesInIL()
{
return _referencesInILMap.GetAllItemsAndCount(out count);
return _referencesInILMap.GetAllItems();
}

/// <summary>
Expand Down

This file was deleted.

133 changes: 133 additions & 0 deletions src/Compilers/Core/Portable/IReferenceOrISignature.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable enable

using System;
using System.Collections.Generic;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.Symbols;

namespace Microsoft.CodeAnalysis
{
/// <summary>
/// Used to devirtualize Dictionary/HashSet for EqualityComparer{T}.Default
/// </summary>
internal struct IReferenceOrISignatureEquivalent : IEquatable<IReferenceOrISignatureEquivalent>
Copy link
Member

@333fred 333fred Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand why we would need both of these: it seems like just IReferenceOrISignatureEquivalent should be fine.

Copy link
Member Author

@benaadams benaadams Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IReferenceOrISignature uses ReferenceEquals for equals and IReferenceOrISignatureEquivalent has a more complex equals, previously provided by MetadataEntityReferenceComparer.cs

The dictionary, hashtable and concurrentdictionary will devirtualize (and potentially inline) the equals for a struct when the Comparer is unspecified (if it also implements IEquatable) in .NET Core; whereas specifying a Comparer will be a slower interface call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great info to have in a comment in the file itself :)


In reply to: 480276103 [](ancestors = 480276103)

benaadams marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly object? _item;
benaadams marked this conversation as resolved.
Show resolved Hide resolved

public IReferenceOrISignatureEquivalent(IReference item)
{
_item = item;
}

public IReferenceOrISignatureEquivalent(ISignature item)
{
_item = item;
}

public IReferenceOrISignatureEquivalent(IMethodReference item)
{
_item = item;
}

public bool Equals(IReferenceOrISignatureEquivalent other)
{
object? x = _item;
object? y = other._item;
if (x is null)
{
return y is null;
}
else if (ReferenceEquals(x, y))
{
return true;
}
benaadams marked this conversation as resolved.
Show resolved Hide resolved

return Equals(x, y);
}

private new static bool Equals(object? x, object? y)
{
if (x is null)
{
return y is null;
}
else if (ReferenceEquals(x, y))
{
return true;
}
else if (x is ISymbolInternal sx && y is ISymbolInternal sy)
{
return sx.Equals(sy, TypeCompareKind.ConsiderEverything);
}
else if (x is ISymbolCompareKindComparableInternal cx && y is ISymbolCompareKindComparableInternal cy)
{
return cx.Equals(cy, TypeCompareKind.ConsiderEverything);
}
else
{
return x.Equals(y);
}
}

public override bool Equals(object? obj)
{
return obj is IReferenceOrISignatureEquivalent refOrSig ?
Equals(refOrSig) :
Equals(_item, obj);
benaadams marked this conversation as resolved.
Show resolved Hide resolved
}

public override int GetHashCode() => _item?.GetHashCode() ?? 0;

public override string ToString() => _item?.ToString() ?? "null";

internal object AsObject() => _item!;
}

/// <summary>
/// Used to devirtualize ConcurrentDictionary for EqualityComparer{T}.Default and ReferenceEquals
/// </summary>
internal struct IReferenceOrISignature : IEquatable<IReferenceOrISignature>
benaadams marked this conversation as resolved.
Show resolved Hide resolved
{
private readonly object? _item;

public static implicit operator IReferenceOrISignature(IReferenceOrISignatureEquivalent d) => new IReferenceOrISignature(d.AsObject());

private IReferenceOrISignature(object? item)
{
_item = item;
}
benaadams marked this conversation as resolved.
Show resolved Hide resolved

public IReferenceOrISignature(IReference item)
{
_item = item;
}

public IReferenceOrISignature(ISignature item)
{
_item = item;
}

public bool Equals(IReferenceOrISignature other)
{
object? x = _item;
object? y = other._item;

return ReferenceEquals(x, y);
}

public override bool Equals(object? obj)
{
return obj is IReferenceOrISignature refOrSig ?
benaadams marked this conversation as resolved.
Show resolved Hide resolved
Equals(refOrSig) :
ReferenceEquals(_item, obj);
}

public override int GetHashCode() => _item?.GetHashCode() ?? 0;

public override string ToString() => _item?.ToString() ?? "null";
}
}
14 changes: 3 additions & 11 deletions src/Compilers/Core/Portable/PEWriter/MetadataWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -464,18 +464,10 @@ private bool IsMinimalDelta

private void CreateMethodBodyReferenceIndex()
{
int count;
var referencesInIL = module.ReferencesInIL(out count);
var referencesInIL = module.ReferencesInIL();

_pseudoSymbolTokenToTokenMap = new EntityHandle[count];
_pseudoSymbolTokenToReferenceMap = new object[count];

int cur = 0;
foreach (object o in referencesInIL)
{
_pseudoSymbolTokenToReferenceMap[cur] = o;
cur++;
}
_pseudoSymbolTokenToTokenMap = new EntityHandle[referencesInIL.Length];
_pseudoSymbolTokenToReferenceMap = referencesInIL;
}

private void CreateIndices()
Expand Down
Loading