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.

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

Cleanup/perf in creating member maps. #67997

Merged
merged 9 commits into from
May 1, 2023
+66 −91
Merged

Cleanup/perf in creating member maps. #67997

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5e19a7f
Cleanup/perf in creating member maps.
CyrusNajmabadi Apr 27, 2023
f51fea2
Simplify
CyrusNajmabadi Apr 27, 2023
f110c34
Merge remote-tracking branch 'upstream/main' into membersCleanup
CyrusNajmabadi Apr 28, 2023
05fa757
Better assert, and avoid empty arrays
CyrusNajmabadi Apr 28, 2023
b11a6c6
Remove type argument
CyrusNajmabadi Apr 28, 2023
7d644dc
Remove unnecessary type arguments
CyrusNajmabadi Apr 28, 2023
876da53
Fix
CyrusNajmabadi Apr 28, 2023
e6b5c2d
Make static
CyrusNajmabadi Apr 28, 2023
8f67daa
Add assert
CyrusNajmabadi Apr 28, 2023
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
97 changes: 36 additions & 61 deletions src/Compilers/Core/Portable/Collections/ImmutableArrayExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Shared.Collections;
using Roslyn.Utilities;

#if DEBUG
using System.Linq;
Expand Down Expand Up @@ -907,45 +908,36 @@ internal static void AddToMultiValueDictionaryBuilder<K, T>(Dictionary<K, object

internal static void CreateNameToMembersMap
<TNamespaceOrTypeSymbol, TNamedTypeSymbol, TNamespaceSymbol>
(Dictionary<string, object> dictionary, Dictionary<String, ImmutableArray<TNamespaceOrTypeSymbol>> result)
(Dictionary<string, object> dictionary, Dictionary<string, ImmutableArray<TNamespaceOrTypeSymbol>> result)
where TNamespaceOrTypeSymbol : class
where TNamedTypeSymbol : class, TNamespaceOrTypeSymbol
where TNamespaceSymbol : class, TNamespaceOrTypeSymbol
{
foreach (var kvp in dictionary)
{
object value = kvp.Value;
ImmutableArray<TNamespaceOrTypeSymbol> members;
result.Add(kvp.Key, createMembers(kvp.Value));

return;

var builder = value as ArrayBuilder<TNamespaceOrTypeSymbol>;
if (builder != null)
static ImmutableArray<TNamespaceOrTypeSymbol> createMembers(object value)
Copy link
Member Author

Choose a reason for hiding this comment

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

moving into local function eliminates complex control flow and tracking of variables. high level algorithm just walks the dictionary, transforming directly into a new dictionary.

{
if (value is ArrayBuilder<TNamespaceOrTypeSymbol> builder)
Copy link
Member Author

Choose a reason for hiding this comment

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

no need for 'hasNamespaces' flag and complex data flow. Once we see a namespace, we know we have to return the builder directly as an ImmutableArray of namespaces or types.

{
Debug.Assert(builder.Count > 1);
bool hasNamespaces = false;
for (int i = 0; i < builder.Count; i++)
foreach (var item in builder)
{
if (builder[i] is TNamespaceSymbol)
{
hasNamespaces = true;
break;
}
if (item is TNamespaceSymbol)
return builder.ToImmutableAndFree();
}

members = hasNamespaces
? builder.ToImmutable()
Copy link
Member Author

Choose a reason for hiding this comment

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

ToImmutable is strictly worse than ToImmutableAndFree. The latter knows hte builder is being freed, so it can directly move the internal array into the final ImmutableArray (if the counts match). The former always has to copy for teh non-zero element case, which is unnecessary when a move is possible.

: ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutable<TNamedTypeSymbol>());

builder.Free();
return ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(builder.ToDowncastedImmutableAndFree<TNamedTypeSymbol>());
Copy link
Member Author

Choose a reason for hiding this comment

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

if we get to end of the loop, we know we didn't have any namespaes, so no need to check any variablie and can just return teh downcasted values.

Copy link
Member

Choose a reason for hiding this comment

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

ImmutableArray.CastUp(builder.ToDowncastedImmutableAndFree())

Is this different from builder.ToImmutableAndFree()?

Copy link
Member Author

Choose a reason for hiding this comment

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

answered offline.

}
else
{
TNamespaceOrTypeSymbol symbol = (TNamespaceOrTypeSymbol)value;
members = symbol is TNamespaceSymbol
? ImmutableArray.Create<TNamespaceOrTypeSymbol>(symbol)
: ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(ImmutableArray.Create<TNamedTypeSymbol>((TNamedTypeSymbol)symbol));
return symbol is TNamespaceSymbol
? ImmutableArray.Create(symbol)
: ImmutableArray<TNamespaceOrTypeSymbol>.CastUp(ImmutableArray.Create((TNamedTypeSymbol)symbol));
}

result.Add(kvp.Key, members);
}
}

Expand All @@ -964,50 +956,33 @@ internal static Dictionary<string, ImmutableArray<TNamedTypeSymbol>> GetTypesFro
{
var dictionary = new Dictionary<string, ImmutableArray<TNamedTypeSymbol>>(comparer);

foreach (var kvp in map)
{
ImmutableArray<TNamespaceOrTypeSymbol> members = kvp.Value;
foreach (var (name, members) in map)
dictionary.Add(name, getOrCreateNamedTypes(members));
Copy link
Member Author

Choose a reason for hiding this comment

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

similarly, top level algorithm is just a walk on the dictionary, producing a new dictionary by transforming the values. I was considering a helper that both methods could call, but i was ok just inlining the foreaches in each. lmk if you'd prefer unification given the closeknit nature of these two methods.


bool hasType = false;
bool hasNamespace = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

no need for any tracking flags, or having to deal with any of the combinations of false/true, true/false, true/true (false/false can't happen as the value is always present and has at least one element it represents)

return dictionary;

foreach (var symbol in members)
ImmutableArray<TNamedTypeSymbol> getOrCreateNamedTypes(ImmutableArray<TNamespaceOrTypeSymbol> members)
{
// See if creator 'map' put a downcasted ImmutableArray<TNamedTypeSymbol> in it. If so, we can just directly
// downcast to that and trivially reuse it. If not, that means the array must have contained at least one
// TNamespaceSymbol and we'll need to filter that out.
var membersAsNamedTypes = members.As<TNamedTypeSymbol>();
Copy link
Member Author

Choose a reason for hiding this comment

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

we know that if we only had named types then CreateNameToMembersMap already put in a downcasted IA, so we can just directly try to move to that. no need for walking the arrays again.


if (!membersAsNamedTypes.IsDefault)
return membersAsNamedTypes;

// Preallocate the right amount so we can avoid garbage reallocs.
var count = members.Count(static s => s is TNamedTypeSymbol);
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
var builder = ArrayBuilder<TNamedTypeSymbol>.GetInstance(count);
foreach (var member in members)
{
if (symbol is TNamedTypeSymbol)
{
hasType = true;
if (hasNamespace)
{
break;
}
}
else
{
#if DEBUG
Debug.Assert(symbol is TNamespaceSymbol);
#endif
hasNamespace = true;
if (hasType)
{
break;
}
}
if (member is TNamedTypeSymbol namedType)
builder.Add(namedType);
}

if (hasType)
{
if (hasNamespace)
{
dictionary.Add(kvp.Key, members.OfType<TNamedTypeSymbol>().AsImmutable());
}
else
{
dictionary.Add(kvp.Key, members.As<TNamedTypeSymbol>());
}
}
Debug.Assert(builder.Count == count);
return builder.ToImmutableAndFree();
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}

return dictionary;
}

internal static bool SequenceEqual<TElement, TArg>(this ImmutableArray<TElement> array1, ImmutableArray<TElement> array2, TArg arg, Func<TElement, TElement, TArg, bool> predicate)
Expand Down
7 changes: 7 additions & 0 deletions src/Dependencies/PooledObjects/ArrayBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,13 @@ public ImmutableArray<U> ToDowncastedImmutable<U>()
return tmp.ToImmutableAndFree();
}

public ImmutableArray<U> ToDowncastedImmutableAndFree<U>() where U : T
{
var result = ToDowncastedImmutable<U>();
this.Free();
return result;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't feel strongly about having this extension. would be ok inlining it. But it fits the pattern of how we use ArrayBuilders for the other ops, so i kept this in sync with those.


/// <summary>
/// Realizes the array and disposes the builder in one operation.
/// </summary>
Expand Down