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

Add API to read, set and ensure capacity of Stack<T> and Queue<T> #44801

Closed
Enderlook opened this issue Nov 17, 2020 · 11 comments · Fixed by #47149
Closed

Add API to read, set and ensure capacity of Stack<T> and Queue<T> #44801

Enderlook opened this issue Nov 17, 2020 · 11 comments · Fixed by #47149
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

@Enderlook
Copy link

Enderlook commented Nov 17, 2020

Background and Motivation

Several dynamic collections offer APIs to check the capacity of their underlying arrays and offer ways to increase it if you know the necessary size ahead of time. For example List<T>.Capacity, HashSet<T>.EnsureCapacity(int) or Dictionary<TKey, TValue>.EnsureCapacity(int).

This allows developers to reserve the necessary space of those collections before performing a heavy workload, and so reduce the number of times those collections must auto-resize, thus reducing GC pressure and CPU time.

However, not all the common collections offer these APIs.

Proposed API

I propose to add the following methods:

namespace System.Collections.Generic
{
     public class Stack<T> {
+	 	public int EnsureCapacity(int capacity);
     }

     public class Queue<T>  {
+	 	public int EnsureCapacity(int capacity);
     }
}

Usage Examples

void FeedWork(T[] workload)
{
    // We know that each work item yields at least 3 results, so we can pre-resize for that
	int minCapacity = this.queue.Count + workload.Length * 3;
	this.queue.EnsureCapacity(minCapacity);

	for (T work in workload)
		for (U obj in GetResults(work))
			this.queue.Enqueue(obj);

	// I made this specific scenario to show that something like queue.EnqueueRange(IEnumerable<U>) wouldn't work here
}

Risks

It increases the surface API.

@Enderlook Enderlook added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 17, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Nov 17, 2020
@ghost
Copy link

ghost commented Nov 17, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

Issue Details
Description:

Background and Motivation

Several dynamic collections offer APIs to check the capacity of their underlying arrays and offer ways to increase it if you know the necessary size ahead of time. For example List<T>.Capacity, HashSet<T>.EnsureCapacity(int) or Dictionary<TKey, TValue>.EnsureCapacity(int).

This allows developers to reserve the necessary space of those collections before performing a heavy workload, and so reduce the number of times those collections must auto-resize, thus reducing GC pressure and CPU time.

However, not all the common collections offer these APIs.

Proposed API

I propose to add .Capacity to Stack<T> and Queue<T>.

namespace System.Collections.Generic
{
     public class Stack<T> {
+	 	public int Capacity { get; set; }
     }

     public class Queue<T> {
+	 	public int Capacity { get; set; }
     }
}

Usage Examples

void FeedWork(T[] workload)
{
    // We know that each work item yields at least 3 results, so we can pre-resize for that
	int minCapacity = this.queue + workload.Length * 3;
	if (this.queue.Capacity < minCapacity)
		this.queue.Capacity = minCapacity;

	for (T work in workload)
		for (U obj in GetResults(work))
			this.queue.Enqueue(obj);

	// I made this specific scenario to show that something like queue.EnqueueRange(IEnumerable<U>) wouldn't work here
}

Alternative Designs

Alternative .EnsureCapacity(int) could be used:

namespace System.Collections.Generic
{
     public class Stack<T> {
+	 	public void EnsureCapacity(int capacity);
     }

     public class Queue<T>  {
+	 	public void EnsureCapacity(int capacity);
     }
}

This APIs has the minor disadvantage that you can't know the current capacity of the underlying array, though that is a less common need.

Using .Capacity will make these collections along the lines of List<T>.
Using .EnsureCapacity(int) will make these collections along the lines of Hashet<T> and Dictionary<TKey, TValue>.
I personally think that a Stack<T> and Queue<T> are more similar to a List<T> -hence my suggestion for .Capacity- rather than a Hashet<T> or Dictionary<TKey, TValue>.Though it can be discussed.

Risks

It increases the surface API.

Author: Enderlook
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Personally, I would prefer the alternative design that you propose for a couple of reasons:

  • A setter that resizes buffers as a side-effect feels counterintuitive to me.
  • Capacity is implementation detail; I don't see why we would need to expose it.

@eiriktsarpalis
Copy link
Member

FWIW I just updated my PriorityQueue API proposal to contain a public void EnsureCapacity(int); method.

@eiriktsarpalis
Copy link
Member

Btw, it seems that the right signature is int EnsureCapacity(int capacity); (cf. #43957 (comment)). Would you be able to update the proposal to only include that design?

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Nov 25, 2020
@Enderlook
Copy link
Author

Btw, it seems that the right signature is int EnsureCapacity(int capacity); (cf. #43957 (comment)). Would you be able to update the proposal to only include that design?

I'm sorry, it's my first time proposing an API. What I'm supposed to do? Edit my initial post to use int EnsureCapacity(int capacity); instead of int Capacity { get; set; }?

@eiriktsarpalis
Copy link
Member

Edit my initial post to use int EnsureCapacity(int capacity); instead of int Capacity { get; set; }?

Yes please :-)

@Enderlook
Copy link
Author

Edit my initial post to use int EnsureCapacity(int capacity); instead of int Capacity { get; set; }?

Yes please :-)

Done

@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 Nov 26, 2020
@eiriktsarpalis eiriktsarpalis self-assigned this Nov 26, 2020
@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Nov 26, 2020
@terrajobst terrajobst 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 Jan 12, 2021
@terrajobst
Copy link
Member

terrajobst commented Jan 12, 2021

Video

  • Looks good
  • Let's also make the method on List<T> public
namespace System.Collections.Generic
{
    public partial class List<T>
    {
        public int EnsureCapacity(int capacity);
    }
    public partial class Stack<T>
    {
        public int EnsureCapacity(int capacity);
    }
    public partial class Queue<T>
    {
        public int EnsureCapacity(int capacity);
    }
}

@eiriktsarpalis eiriktsarpalis added the help wanted [up-for-grabs] Good issue for external contributors label Jan 12, 2021
@lateapexearlyspeed
Copy link
Contributor

Hi @eiriktsarpalis, this was assigned to you but mark as up for grabs, so not sure can external guys pick to implement or not?

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jan 18, 2021

Hi @lateapexearlyspeed I had myself assigned for API review purposes. Yes this would be open to external contributes, would you be interested in taking this? :-)

@eiriktsarpalis eiriktsarpalis removed their assignment Jan 18, 2021
@lateapexearlyspeed
Copy link
Contributor

@eiriktsarpalis Yes, I would try this, thanks :)

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 19, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Feb 4, 2021
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Feb 8, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
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