Runtime: ArrayPool perf could be improved with non-zeroed large array allocations

[JeremyKuhne]

For performance reasons we don't clear arrays when returned unless specifically requested:

C# public abstract class ArrayPool<T> { public abstract void Return(T[] array, bool clearArray = false); }

Zeroing out of new arrays is a cost that isn't necessary. Any time we can avoid this would be beneficial- there are two issues that track adding this sort of functionality to the GC:

https://github.com/dotnet/coreclr/issues/17324
https://github.com/dotnet/coreclr/issues/17325

If we get that functionality we should update ArrayPool to utilize it.

cc: @Maoni0

[morganbr]

@GrabYourPitchforks has been looking at things like this since not zeroing has security implications.

[JeremyKuhne]

@GrabYourPitchforks has been looking at things like this since not zeroing has security implications.

Note that we currently don't zero buffers that are returned to ArrayPool unless requested. That said, I do recognize that it isn't exactly the same to not zero newly allocated buffers.

[GrabYourPitchforks]

And I've been pushing back against the existing behavior because I've seen cases where developers assumed the returned arrays were zero-inited even though they weren't. :)

My ideal scenario would be to have two different array pools - one which guarantees zero-init on rental (and which is used by most consumers), and one which doesn't. The latter could take advantage of proposed GC features which avoid zero-initing heap memory. Usage of the second pool would require the same type of review which would be given to any unsafe-equivalent code.

You could even name the array pools ArrayPool<T>.Shared and ArrayPool<T>.Unsafe, so they'd both present themselves through the same API.

[JeremyKuhne]

The current one is already unsafe by definition for multiple reasons. One is the lack of zero init- the other is that you can return a buffer without letting go of your handle. I don't see how you can get around that and bifurcate "safe/unsafe".

I can see adding a rent overload that zeroes. ArrayPool.Rent(int minimumLength, bool clearArray).

[GrabYourPitchforks]

You're right in that it's not guaranteed "safe" from a threading perspective, but honestly I don't think use-after-release is a very big problem with ArrayPool<T>. I _do_ think developers are likely to rent buffers without knowing the implications of what they're doing, mainly because as mentioned in other threads I've seen evidence of this and have come across code which assumes zero-init. The fact that Span<T> span = stackalloc T[...]; is essentially guaranteed (subject to some very weird edge cases) to zero-init also plays in here.

I believe the current API is misdesigned and subjects our developer audience to a pit of failure. I believe we should take the perf hit of changing the behavior of the existing APIs and should introduce a new sister API (whether it be an overload or whatever) that allows developers to opt out of zero-init. And I would be against changing the behavior of ArrayPool<T>.Rent to return arrays that might contain data like object headers and other random pointers without having the caller explicitly opt in to this behavior.

[JeremyKuhne]

I don't think use-after-release is a very big problem

I'm _very_ worried about double-release or use-after-release. :)

I do think developers are likely to rent buffers without knowing the implications of what they're doing

I think they'd be less likely to if we had intellisense dangling the clearArray signature on it.

The fact that Span span = stackalloc T[...]; is essentially guaranteed (subject to some very weird edge cases) to zero-init

Unless you opt out of it, of course. ;P https://github.com/dotnet/corefx/pull/26993

I believe we should take the perf hit of changing the behavior of the existing APIs and should introduce a new sister API (whether it be an overload or whatever) that allows developers to opt out of zero-init.

That would make for some very unhappy people in 2.2 I would assume.

I would be against changing the behavior of ArrayPool.Rent to return arrays that might contain data from random managed heap memory without requiring the caller to explicitly opt in to this behavior.

I'd be fine with that being an explicit opt-in as well.

On a related note we should look closer at adding diagnostic helpers for the ArrayPools. I don't know that we have an issue that covers this already. It would be nice, for example, to have a mode that fills the memory with 0x0ABBACAB or something for testing.

[GrabYourPitchforks]

I'm very worried about double-release or use-after-release. :)

Never said it _wasn't_ a problem, just that it seemingly pales in comparison to other misuses of ArrayPool I've come across.

That would make for some very unhappy people in 2.2 I would assume.

I don't think developers really need fear performance regressions. I ran the following sample application with the specified line both commented and uncommented and saw only a 1% performance difference between the two runs.

var sw = new Stopwatch();
var rnd = new Random();
while (true)
{
    sw.Restart();
    for (int j = 0; j < NUM_ITERS; j++)
    {
        var chars = ArrayPool<char>.Shared.Rent(16);
        // ((Span<char>)chars).Clear(); // uncomment this line
        rnd.Next().TryFormat(chars, out _);
        ArrayPool<char>.Shared.Return(chars);
    }
    Console.WriteLine(sw.ElapsedMilliseconds);
}

But even then, for extremely perf-sensitive code paths, the "just give me raw uninitialized memory" APIs would still exist.

It would be nice, for example, to have a mode that fills the memory with 0x0ABBACAB or something for testing.

This is already being tracked at https://github.com/dotnet/coreclr/issues/17253.

[stephentoub]

I continue to believe we should not change the clearing behavior of the existing API. Full stop.

[GrabYourPitchforks]

And I think this particular work item should be closed as won't fix. Full stop.

(Seriously, though, we need to pay much more attention to usability _during_ the API design process going forward. In recent months there have been a lot of last-minute panics due to us discovering very late in the development cycle that proposed or shipping APIs don't actually behave in our customers' best interests.)

[JeremyKuhne]

Never said it wasn't a problem, just that it seemingly pales in comparison to other misuses of ArrayPool I've come across.

Not that I don't think you didn't think it wasn't an issue- I'm just more concerned about that, comparatively. The sort of problem you get from presuming zeroed is one that I don't think you'll get very far with before you fall over and learn your lesson. It isn't nearly as chill inducing to me as having bytes change out from under you randomly.

I ran the following sample application with the specified line both commented and uncommented and saw only a 1% performance difference between the two runs.

If all we were talking about were 16 element buffers I wouldn't be concerned about the perf impacts. I'm much more concerned about the larger buffers where initialization isn't trivial and starts causing things like unnecessary page faults. A typical "larger"usage is the 4K char or byte buffer. These buffers can get much larger than that, of course.

[VSadov]

I think this is resolved in https://github.com/dotnet/coreclr/pull/24504