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

Optimize ArrayBuilder.ToDictionary to use SmallDictionary when appropriate #69049

Open
Youssef1313 opened this issue Jul 15, 2023 · 3 comments
Open

Comments

@Youssef1313
Copy link
Member

internal Dictionary<K, ImmutableArray<T>> ToDictionary<K>(Func<T, K> keySelector, IEqualityComparer<K>? comparer = null)
where K : notnull
{
if (this.Count == 1)
{
var dictionary1 = new Dictionary<K, ImmutableArray<T>>(1, comparer);
var value = this[0];
dictionary1.Add(keySelector(value), ImmutableArray.Create(value));
return dictionary1;
}
if (this.Count == 0)
{
return new Dictionary<K, ImmutableArray<T>>(comparer);
}
// bucketize
// prevent reallocation. it may not have 'count' entries, but it won't have more.
var accumulator = new Dictionary<K, ArrayBuilder<T>>(Count, comparer);
for (var i = 0; i < Count; i++)
{
var item = this[i];
var key = keySelector(item);
if (!accumulator.TryGetValue(key, out var bucket))
{
bucket = ArrayBuilder<T>.GetInstance();
accumulator.Add(key, bucket);
}
bucket.Add(item);
}
var dictionary = new Dictionary<K, ImmutableArray<T>>(accumulator.Count, comparer);
// freeze
foreach (var pair in accumulator)
{
dictionary.Add(pair.Key, pair.Value.ToImmutableAndFree());
}
return dictionary;
}

  1. The method should return IDictionary and return SmallDictionary for small number of elements where it will be more efficient.
  2. If there are no elements, I don't think it should allocate a new dictionary every time. If it will return IDictionary<TKey,ImmutableArray<TValue>>, then we can just use ImmutableDictionary<K, ImmutableArray<T>>.Empty. Otherwise, create our own cached empty.

Motivation:

image

It looks like GetSimpleNonTypeMembers has 7.7% of CPU time in this trace, with most of its time in TryGetValue. I'd imagine that there are some of these calls where the number of elements in the dictioanry is small enough that SmallDictionary can bring a better performance.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 15, 2023
@jaredpar
Copy link
Member

I feel like we're not getting a lot of context on these issues. I appreciate there is a screen shot of PerfView here but there isn't surrounding context. I don't know what you're measuring, how that scenario relates to other work we're doing, etc ...

@Youssef1313
Copy link
Member Author

@jaredpar The scenario is referencing a very large dll with too many generic extension methods. The dll is about 9MB, and it's only code.

@Youssef1313
Copy link
Member Author

Youssef1313 commented Jul 18, 2023

What I'm measuring is IntelliSense performance. Basically just start collecting a trace and start typing in IDE for a while. That's it. IntelliSense and classification appear to be both slow. Most of the time is spent in the compiler, so it doesn't appear to be an IDE issue.

The semantic model calls get veery slow with too many generic extension methods (there are ones with multiple overloads) in metadata symbols.

A similar scenario existed for retargeting symbols, which has improved in #69011

@jcouv jcouv added this to the Backlog milestone Jul 19, 2023
@jcouv jcouv added help wanted The issue is "up for grabs" - add a comment if you are interested in working on it and removed untriaged Issues and PRs which have not yet been triaged by a lead help wanted The issue is "up for grabs" - add a comment if you are interested in working on it labels Jul 19, 2023
@jcouv jcouv modified the milestones: Backlog, 17.9 Jul 19, 2023
@jaredpar jaredpar modified the milestones: 17.9, Backlog Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants