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]: ImmutableArray<T>.Builder.ToImmutableAndClear() #72625

Closed
Tracked by #77391
333fred opened this issue Jul 21, 2022 · 6 comments · Fixed by #79385
Closed
Tracked by #77391

[API Proposal]: ImmutableArray<T>.Builder.ToImmutableAndClear() #72625

333fred opened this issue Jul 21, 2022 · 6 comments · Fixed by #79385
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@333fred
Copy link
Member

333fred commented Jul 21, 2022

Background and motivation

In basically every project I use ImmutableArray in, I end up creating an extension for ToImmutableAndClear on the builder type, which takes advantage of MoveToImmutable() if the current builder has right size, and otherwise falls back to ToImmutable()/Clear() if not. I would really love it if we added this to the BCL itself.

API Proposal

namespace System.Collections.Immutable;

public struct ImmutableArray<T>
{
    public class Builder
    {
        public ImmutableArray<T> ToImmutableAndClear()
        {
            if (Capacity == Count) return MoveToImmutable();
            else
            {
                var result = ToImmutable();
                Clear();
                return result;
            }
       }
    }
}

API Usage

var builder = ImmutableArray.CreateBuilder<string>(/* max possible values */ 42);

foreach (var x in thingGettingMapped)
{
    if (x.Frob) builder.Add(x);
}

return builder.ToImmutableAndClear();

Alternative Designs

No response

Risks

No response

@333fred 333fred added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 21, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 21, 2022
@ghost
Copy link

ghost commented Jul 21, 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

In basically every project I use ImmutableArray in, I end up creating an extension for ToImmutableAndClear on the builder type, which takes advantage of MoveToImmutable() if the current builder has right size, and otherwise falls back to ToImmutable()/Clear() if not. I would really love it if we added this to the BCL itself.

API Proposal

namespace System.Collections.Immutable;

public struct ImmutableArray<T>
{
    public class Builder
    {
        public ImmutableArray<T> ToImmutableAndClear()
        {
            if (Capacity == Count) return MoveToImmutable();
            else
            {
                var result = ToImmutable();
                Clear();
                return result;
            }
       }
    }
}

API Usage

var builder = ImmutableArray.CreateBuilder<string>(/* max possible values */ 42);

foreach (var x in thingGettingMapped)
{
    if (x.Frob) builder.Add(x);
}

return builder.ToImmutableAndFree();

Alternative Designs

No response

Risks

No response

Author: 333fred
Assignees: -
Labels:

api-suggestion, area-System.Collections

Milestone: -

@Sergio0694
Copy link
Contributor

I also use something like this quite often in my generators and would very much welcome this API 🙂

Nit: the proposal says ToImmutableAndClear but the example uses ToImmutableAndFree, old snippet?

@333fred
Copy link
Member Author

333fred commented Jul 21, 2022

Nit: the proposal says ToImmutableAndClear but the example uses ToImmutableAndFree, old snippet?

No, force of habit. Roslyn also pools builders.

@eiriktsarpalis
Copy link
Member

Sounds useful enough. If we do add such a method, we might want to explicitly document that any perf benefits are predicated on Capacity == Count.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Jul 22, 2022
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Jul 22, 2022
@eiriktsarpalis eiriktsarpalis added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 22, 2022
@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

The challenge is that MoveToImmutable() results in Capacity becoming 0, while ToImmutable(); Clear(); would preserve the capacity. This can result in cases where we need go through various resize cycles, making the Capacity == Count potentially more expensive.

namespace System.Collections.Immutable;

public partial struct ImmutableArray<T>
{
    public partial class Builder
    {
        public ImmutableArray<T> ToImmutableAndClear();
    }
}

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Aug 16, 2022
@bartonjs
Copy link
Member

bartonjs commented Sep 6, 2022

Video

  • ToImmutableAndClear, as proposed, has a problem where when Capacity==Count Capacity changes to 0, and otherwise it remains.
  • We instead think the "do the cheapest thing possible" would be DrainToImmutable, always making Capacity 0 as a result of being called.
    • We debated many verbs (Drain, Extract, Empty) and prefix vs post-fix, but prefix matched the existing MoveToImmutable vs ToImmutable choice.
namespace System.Collections.Immutable;

public struct ImmutableArray<T>
{
    public partial class Builder
    {
        public ImmutableArray<T> DrainToImmutable();
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 6, 2022
@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Oct 7, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 8, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Collections help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants