dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.65k forks source link

Consider introducing an overload of `ArrayPool<T>.Create` which allows for default pinning #36183

Open john-h-k opened 4 years ago

john-h-k commented 4 years ago

Given we have the new POH, this seems both quite trivial and quite powerful

public class ArrayPool<T>
{
    public static ArrayPool<T> Create(bool pinned);
}

In terms of implementation, it seems to simply be about holding the necessary state in ConfigurableArrayPool and just replacing new T[] with GC.AllocateArray

ghost commented 4 years ago

Tagging subscribers to this area: @tannergooding Notify danmosemsft if you want to be subscribed.

EgorBo commented 4 years ago

Related: https://github.com/dotnet/runtime/pull/33738 (see discussion)

john-h-k commented 4 years ago

So it looks like, as reasonably expected, POH does have some overhead. However, I think that is to be expected, and the manual-ness of this means it isn't an issue. Obviously as a new API, there isn't a perf regression

The one thing I could see is actually creating a seperate type, to prevent having to take the slow alloc path in ConfigurableArrayPool.

There could also possibly be

ArrayPool<T>.SharedPinned
GrabYourPitchforks commented 4 years ago

Behavioral question: would returning a T[] to this pool need to validate whether the array instance does in fact belong to the POH? It could destabilize the application were somebody to return a non-pinned array to this pool. But I don't know how likely it would be for somebody actually to do that.

john-h-k commented 4 years ago

That is a use case for #33542

But given invalidly returning buffers already borks the shared arraypool implementation iirc, i don't think it would be too awful to leave it as a non-POH array to simply bork

GrabYourPitchforks commented 4 years ago

But given invalidly returning buffers already borks the shared arraypool implementation iirc, i don't think it would be too awful to leave it as a non-POH array to simply bork

I can see that, but improperly returning an array to the shared array pool horks it in a different way. In that scenario, the biggest concern is use-after-release, so two consumers might end up operating on the same buffer instance. But aside from them stomping on each other's data they're guaranteed to remain within the bounds of the array instance.

If you return a non-POH array to the pinned array pool, you might AV the process at some indeterminate point in the future due to an out-of-bounds read or write to the array. (The reasoning is that the scenario is that the consumer of the POH assumes the array instances are already pinned, so they might bypass pinning themselves when attempting to operate on any rented instance.)

john-h-k commented 4 years ago

Hmm, thats a fair point. You could maintain a hashset of the dictionaries internally optionally, and that could be another parameter provided (bool trackArrays = true)

davidfowl commented 4 years ago

Another case for https://github.com/dotnet/runtime/issues/33542

EgorBo commented 4 years ago

Hmm, thats a fair point. You could maintain a hashset of the dictionaries internally optionally, and that could be another parameter provided (bool trackArrays = true)

Or just set a bit in the object header 🙂 something like this: https://github.com/EgorBo/coreclr/commit/dba0a10e3257db975cc98a88b03f46bd7cf88eac