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

[API Proposal]: Additional ToFrozenDictionary overloads with optimizeForReading #87633

Closed
BrennanConroy opened this issue Jun 15, 2023 · 2 comments · Fixed by #87988
Closed
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Milestone

Comments

@BrennanConroy
Copy link
Member

Background and motivation

When experimenting with using FrozenDictionary in dotnet/aspnetcore#47569, we had to write
entries.ToDictionary(x => x.text, x => x.destination).ToFrozenDictionary(StringComparer.OrdinalIgnoreCase, true);
to get an optimized FrozenDictionary when ideally it would have been
entries.ToFrozenDictionary(x => x.text, x => x.destination, StringComparer.OrdinalIgnoreCase, true);

This matches the other overload of ToFrozenDictionary that takes an IEnumerable<KeyValuePair<TKey, TValue>>.

API Proposal

public static partial class FrozenDictionary
{
    public static FrozenDictionary<TKey, TValue> ToFrozenDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
    public static FrozenDictionary<TKey, TValue> ToFrozenDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
    public static FrozenDictionary<TKey, TSource> ToFrozenDictionary<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
+   public static FrozenDictionary<TKey, TSource> ToFrozenDictionary<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
    public static FrozenDictionary<TKey, TElement> ToFrozenDictionary<TSource, TKey, TElement>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
+   public static FrozenDictionary<TKey, TElement> ToFrozenDictionary<TSource, TKey, TElement>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
}

API Usage

entries.ToFrozenDictionary(x => x.text, x => x.destination, StringComparer.OrdinalIgnoreCase, true);

Alternative Designs

No response

Risks

More API surface.

@BrennanConroy BrennanConroy added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 15, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 15, 2023
@ghost
Copy link

ghost commented Jun 15, 2023

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

When experimenting with using FrozenDictionary in dotnet/aspnetcore#47569, we had to write
entries.ToDictionary(x => x.text, x => x.destination).ToFrozenDictionary(StringComparer.OrdinalIgnoreCase, true);
to get an optimized FrozenDictionary when ideally it would have been
entries.ToFrozenDictionary(x => x.text, x => x.destination, StringComparer.OrdinalIgnoreCase, true);

This matches the other overload of ToFrozenDictionary that takes an IEnumerable<KeyValuePair<TKey, TValue>>.

API Proposal

public static partial class FrozenDictionary
{
    public static FrozenDictionary<TKey, TValue> ToFrozenDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
    public static FrozenDictionary<TKey, TValue> ToFrozenDictionary<TKey, TValue>(this IEnumerable<KeyValuePair<TKey, TValue>> source, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
    public static FrozenDictionary<TKey, TSource> ToFrozenDictionary<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
+   public static FrozenDictionary<TKey, TSource> ToFrozenDictionary<TSource, TKey>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
    public static FrozenDictionary<TKey, TElement> ToFrozenDictionary<TSource, TKey, TElement>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector, IEqualityComparer<TKey>? comparer = null) where TKey : notnull;
+   public static FrozenDictionary<TKey, TElement> ToFrozenDictionary<TSource, TKey, TElement>(this IEnumerable<TSource> source, Func<TSource, TKey> keySelector, Func<TSource, TElement> elementSelector, IEqualityComparer<TKey>? comparer, bool optimizeForReading) where TKey : notnull;
}

API Usage

entries.ToFrozenDictionary(x => x.text, x => x.destination, StringComparer.OrdinalIgnoreCase, true);

Alternative Designs

No response

Risks

More API surface.

Author: BrennanConroy
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@stephentoub
Copy link
Member

While I'm the one who pushed for us to add optimizeForReading, I'd still really like to see if we can do away with it before we actually ship. It's there because a) the creation costs are so massive when compared to dictionary that anyone choosing to use the type for its surface area rather than many, many reads over the lifetime of the app will find themselves regressing perf, and b) for native aot carrying around all the different specializations for each TKey/TValue leads to asm bloat. However, @adamsitnik is currently whittling down the costs; if we got them down sufficiently, it'd be nice to not force extra work on behalf of devs to achieve the primary reason these types were initially created. That said, if we don't get there and we need to ship with the flag still in place, adding the additional overloads is reasonable.

@stephentoub stephentoub added this to the 8.0.0 milestone Jun 15, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 15, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 23, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 26, 2023
@eiriktsarpalis eiriktsarpalis closed this as not planned Won't fix, can't repro, duplicate, stale Jun 26, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants