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]: ImmutableDictionary.SetItems(params ValueTuple<TKey, TValue>[] Values) #71092

Closed
TonyValenti opened this issue Jun 21, 2022 · 8 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections needs-author-action An issue or pull request that requires more info or actions from the author.
Milestone

Comments

@TonyValenti
Copy link

Background and motivation

It would be really nice if the SetItems API allowed me to use params and value tuples to easily specify items that need to be updated.

API Proposal

namespace System.Collections.Generic;

public class ImmutableDictionary<TKey, TValue>
{
    public ImmutableDictionary<TKey, TValue> SetItems(params ValueTuple<TKey, TValue>[] Values);
}

API Usage

var NewValues = OldValues.SetItems(
                    (DateFieldName.Path_CreatedAt, NewCreatedAt),
                    (DateFieldName.Path_UpdatedAt, NewUpdatedAt),
                    (Type_CreatedAt, NewCreatedAt),
                    (Type_UpdatedAt, NewUpdatedAt)
                );

Alternative Designs

This could be made an extension method or left out altogether.

Risks

None that I can think of.

@TonyValenti TonyValenti added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 21, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 21, 2022
@ghost
Copy link

ghost commented Jun 22, 2022

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

It would be really nice if the SetItems API allowed me to use params and value tuples to easily specify items that need to be updated.

API Proposal

namespace System.Collections.Generic;

public class ImmutableDictionary<TKey, TValue>
{
    public ImmutableDictionary<TKey, TValue> SetItems(params ValueTuple<TKey, TValue>[] Values);
}

API Usage

var NewValues = OldValues.SetItems(
                    (DateFieldName.Path_CreatedAt, NewCreatedAt),
                    (DateFieldName.Path_UpdatedAt, NewUpdatedAt),
                    (Type_CreatedAt, NewCreatedAt),
                    (Type_UpdatedAt, NewUpdatedAt)
                );

Alternative Designs

This could be made an extension method or left out altogether.

Risks

None that I can think of.

Author: TonyValenti
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@TonyValenti
Copy link
Author

@jeffhandley

@danmoseley
Copy link
Member

Have you thought about how often this would be used? Eg perhaps some looking in https://Grep.app ... as you know, there needs to be sufficient need to add a new API, particularly when it can be a simple extension method. Is there a reason this would be particularly valuable on ImmutableDictionary (eg perf) - it does not exist on Dictionary<K,V> which has far more usage.

@TonyValenti
Copy link
Author

Hi @danmoseley -
With ImmutableDictionary, whenever the dictionary is modified, there is overhead. By adding multiple items at once, you only have to pay the tax once. (At least that is my assumption about how it works. I could be wrong!)

With dictionary, the overhead is constant regardless of how many items are added.

@svick
Copy link
Contributor

svick commented Jun 28, 2022

@TonyValenti You can largely achieve the same effect yourself by writing an extension method that uses the dictionary's builder:

public static ImmutableDictionary<TKey, TValue> SetItems<TKey, TValue>(
    this ImmutableDictionary<TKey, TValue> dictionary, params ValueTuple<TKey, TValue>[] values)
{
    var builder = dictionary.ToBuilder();
    
    foreach (var (key, value) in values)
    {
        builder[key] = value;
    }

    return builder.ToImmutable();
}

When I compare this using a simple benchmark based on your example code, the results I get are:

Method Mean Error StdDev Gen 0 Allocated
OneByOne 1.318 us 0.0098 us 0.0092 us 0.2136 672 B
SetItems 1.058 us 0.0105 us 0.0093 us 0.1316 416 B
ExtensionSetItems 1.128 us 0.0104 us 0.0092 us 0.1392 440 B

I.e. setting the items one by one is indeed relatively bad, especially when it comes to allocations. But an extension SetItems using Builder is comparable with the existing overload of SetItems.

@krwq
Copy link
Member

krwq commented Jul 7, 2022

@TonyValenti does @svick sound like a reasonable solution? If so can this issue be closed? I'm also personally not a fan of adding helper methods for every possible use case

@krwq krwq added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jul 7, 2022
@ghost
Copy link

ghost commented Jul 7, 2022

This issue has been marked needs-author-action and may be missing some important information.

@krwq krwq added this to the Future milestone Jul 7, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2022
@TonyValenti TonyValenti closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
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 needs-author-action An issue or pull request that requires more info or actions from the author.
Projects
None yet
Development

No branches or pull requests

5 participants