Allow for ArrayPool<T> to create buffers with a different minimum length

[xoofx]

Related to PR dotnet/corefx#12642

Problem

Currently, the default minimum length that can be rented from an ArrayPool<T> is hardcoded to 16 in a few places, in DefaultArrayPool.cs and in Utilities.cs.

Typically I would like to use ArrayPool for arrays allocated for a custom List type and I would like to use it for a minimum size of 4 (instead of 16), but this could go as low as 1.

Proposal

The proposal is to add a new method public method ArrayPool<T>.Create(minLength, maxLength, maxArrayPerBucket) to allow such a scenario and modify the underlying code accordingly.

[stephentoub]

cc: @KrzysztofCwalina, @sokket

[terrajobst]

@xoofx

Adding this new API seems reasonable to us:

 public static class ArrayPool<T>
 {
     public static ArrayPool<T> Create();
     public static ArrayPool<T> Create(int maxArrayLength, int maxArraysPerBucket);
+    public static ArrayPool<T> Create(int minArrayLength, int maxArrayLength, int maxArraysPerBucket);
 }

Open issues:

• Right now they are many assumptions in the implementation. We should make sure that the interdependence doesn't regress other behavior, such as perf.
• Should minArrayLength required to be a power of 2? Seems the current implementation depends on that.
• Do you have any expectations for the static shared pool?

[xoofx]

Should minArrayLength required to be a power of 2? Seems the current implementation depends on that.

I didn't want to throw an exception, so the code behind will round to the upper closest power of 2 . Apart maybe to improve the doc, not sure what's best in this case?

Do you have any expectations for the static shared pool?

I left as it as it was, nothing changed. Doesn't fit well with a plain static access (and without C++ templates it is not possible)

though @stephentoub mentioned that there might be implementation reasons for the original default size.

[alexperovich]

@terrajobst Is this api ready to implement provided:

• The shared pool isn't changed
• minArrayLength is rounded up to the next power of 2
• The performance remains the same or better

[karelz]

@alexperovich you should mark it as api-ready-for-review, if you think it is ready for review ...

[terrajobst]

OK, in that case it's good to go.

[alexperovich]

@xoofx Are you working on this?

[xoofx]

@alexperovich sure, I can rebuild my PR, add some tests, hopefully later this week

[karelz]

Just curious how things are going here ... anything we can do to help?

[danmoseley]

@MaggieTsang @niustephanie @nchikanov this is up for grabs, if one of you would like to take it, please assign to yourself.

[KrzysztofCwalina]

If we add this API, I would prefer the new parameter to be last. i.e.

public static ArrayPool<T> Create(int maxArrayLength, int maxArraysPerBucket, int minArrayLength);

[karelz]

Unassigned @xoofx and marked again as up-for-grabs - anyone interested?

[danmoseley]

@JeremyKuhne fyi a possible intern project.

[danmoseley]

I would prefer the new parameter to be last. i.e.
public static ArrayPool Create(int maxArrayLength, int maxArraysPerBucket, int minArrayLength);

I agree, I think this was an oversight during API review. We should not reorder the paramters.

[kevingosse]

I don't mind taking care of it, though it would mostly be copy&pasting from @xoofx's pull request (it looks pretty good) + test coverage. If @JeremyKuhne or anybody else has special interest for it, I don't mind leaving it either.

I agree, I think this was an oversight during API review. We should not reorder the paramters.

But that means putting the "min" parameter after the "max". Looks error-prone to me.

[danmoseley]

@terrajobst can you please comment on the ordering of min and max? Typically min comes before max, but also typically new parameters go on the end, not at the start:

     public static ArrayPool<T> Create(int maxArrayLength, int maxArraysPerBucket);
+    public static ArrayPool<T> Create(int minArrayLength, int maxArrayLength, int maxArraysPerBucket);

[KrzysztofCwalina]

I think switching the order of an int parameter is at least equally error prone.

From our design guidelines:
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/member-overloading

X AVOID being inconsistent in the ordering of parameters in overloaded members. Parameters with the same name should appear in the same position in all overloads.

[kevingosse]

I did a cursory search on CoreFX to find similar min/max overloads, surprisingly it's a very rare situation. Still, I could find Random.Next:

public partial class Random
{
     public virtual int Next(int maxValue) { throw null; }
     public virtual int Next(int minValue, int maxValue) { throw null; }
}

[danmoseley]

Marking for review again to settle on parameter ordering

[terrajobst]

Video

The ordering is unfortunate. @KrzysztofCwalina mentioned that we might want to take more options in the future to customize how the sizes are spread inside the pool; this indicates that we probably shouldn't take more values as individual arguments but instead introduce a new class that holds the the options like this:

public class ArrayPoolOptions
{
    public int MaxArrayLength { get; set; }
    public int MaxArraysPerBucket { get; set; }
    public int MinArrayLength { get; set; }
}

public static class ArrayPool<T>
{
    public static ArrayPool<T> Create();
    public static ArrayPool<T> Create(int maxArrayLength, int maxArraysPerBucket);
    public static ArrayPool<T> Create(ArrayPoolOptions options);
}

(this needs more design, but illustrates the point)

[benaadams]

public class ArrayPoolOptions
{
    public Func<int, T[]> CreateBucketSize { get; set; }
}

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/BI3iXFT8H7E?t=953 (13 min duration)

[AArnott]

I'm interested in this as well, but not so that I can decrease the min size as the issue description says, but rather so I can increase the min size.

In serialization scenarios where IBufferWriter<T>.GetSpan(int) is called frequently with very small values (e.g. 4, 12), this can result in a serialization process that allocates many thousands of small arrays. When the scenario owner is aware that there will be many small writes, it might initialize the ArrayPool/MemoryPool used in the IBufferWriter<T>, instructing it to never return arrays with length less than some integer.

When trying this locally, this reduced a serialization from tens of thousands of 32 byte arrays to just a handful of 4K arrays.

It seems that this could also be done at the IBufferWriter<T> implementation level (to ensure that only larger arrays are allocated), but doing it at the pool level would make it more generally useful, IMO.