Runtime: Harvest System.Buffers for NS2 and downlevel

[qbit86]

Browsing sources in this repo I came across two versions of abstract class ArrayPool<T>: src/System.Buffers/src/System/Buffers/ArrayPool.cs and src/Common/src/CoreLib/System/Buffers/ArrayPool.cs.
And its three implementations: DefaultArrayPool<T> and TlsOverPerCoreLockedStacksArrayPool<T> + ConfigurableArrayPool<T>.

Why are there two versions of it? Which one is actually used and under what circumstances?

/cc @ianhays @safern

[karelz]

System.Buffers is the one used publicly. The one in CoreLib is shared with CoreCLR as source code and used there for layering purposes.

[qbit86]

As far as I can see, System.Buffers NuGet package contains version based on DefaultArrayPool<T> from System.Buffers/. And .NET Core implementation uses TlsOverPerCoreLockedStacksArrayPool<T> from CoreLib/:

Console.WriteLine(ArrayPool<byte>.Shared.GetType().FullName);

But why are two different implementations needed?

I'm not sure if I understand implementations correctly. From what I see, TlsOverPerCoreLockedStacksArrayPool<T> uses ConditionalWeakTable<...> while ConfigurableArrayPool<T> and DefaultArrayPool<T> don't use any weak references or other interaction with GC. So if we Return() a lot, these pools keep holding references to buffers even in case of memory shortage and garbage collection, aren't they? Doesn't this effectively mean kind of memory leak?

(There is label area-System.Buffers, maybe it is applicable here.)

Thanks for clarification!

[danmosemsft]

@qbit86 DefaultArrayPool limits the buffers it caches (numbers of them and sizes of them). So it chooses to allow bounded memory accumulation. Other schemes are possible, for example, you could time out buffers. I guess ArrayPool is abstract so that it is possible to make other choices.

[danmosemsft]

I suspect that the duplication between ConfigurableArrayPool and DefaultArrayPool is not intentional (their code is very similar and filenames are different). There are some cases where we have duplicated types into corelib which as @karelz says was because we needed some form of it (usually a cut down one) at a low layer but wanted the publicly used one to be in corefx to be easily improved. Dictionary used to be in this case. I think we are trying to avoid this and it's less necessary than it was.

@jkotas @JeremyKuhne any reason why System.Buffers couldn't use src\Common\src\CoreLib\System\Buffers\ConfigurableArrayPool.cs and delete DefaultArrayPool? Although I notice that ConfigurableArrayPool is in corelib even though nothing in corelib actually uses it (nobody does ArrayPool<T>.Create).

It looks like everyone else uses ArrayPool.Shared which is TlsOverPerCoreLockedStacksArrayPool. I assume Buffers specially does not because it needs different performance characteristics.

[safern]

Basically the differences I see between System/Buffers/ArrayPool.cs vs CoreLib/System/Buffers/ArrayPool.cs is that the first (corefx) uses DefaultArrayPool as the shared instance. Coreclr's ArrayPool.Shared returns an instance of TlsOverPerCoreLockedStacksArrayPool<T>.

It is worth to note that corefx's ArrayPool implementation is only used for netstandard, netstandard1.1 and uap10.0.16299 -- https://github.com/dotnet/corefx/blob/master/src/System.Buffers/src/System.Buffers.csproj#L6

When we build for netcoreapp we typeforward to System.Private.CoreLib's implementation of ArrayPool. I guess this pattern was used because netstandard isn't a runtime but a library target and this corefx's implementation is left simple to what is available in netstandard1.1 and uap10.0.16299.

[jkotas]

Note that System.Buffers is set in stone because of it was pulled into netcoreapp. We may want to stop building the downlevel versions of System.Buffers and just pick them up via harvesting.

[safern]

We may want to stop building the downlevel versions of System.Buffers and just pick them up via harvesting.

Still we would need a nestandard implementation right? (To work on netfx and/or uap).

[JeremyKuhne]

The duplication between ConfigurableArrayPool and DefaultArrayPool is likely fallout of our older processes. We didn't have a way to effectively share code between the repos when this was written.Afaik we can remove DefaultArrayPool for clarity. (Either by just using the mirrored code or harvesting as @jkotas discussed.)

TlsOverPerCoreLockedStacksArrayPool<T> utilizes functionality that is only in .NET Core (Thread.GetCurrentProcessorId for one, in addition to some GC internals.) It is only used for Shared as it is tuned for a very specific setup- and wouldn't work against the configuration options you get when not using Shared.

When TlsOverPerCoreLockedStacksArrayPool<T> was introduced for perf, the copy of DefaultArrayPool was renamed to ConfigurableArrayPool as I recall. Unfortunately the history is a little hard to follow due to the way file moves were made. I'll find the change and tag it here.

[JeremyKuhne]

Here is where it was renamed: https://github.com/dotnet/coreclr/pull/8209

[jkotas]

Still we would need a nestandard implementation right? (To work on netfx and/or uap).

nestandard can come via harvesting too. uap has its own build that is same as netcoreapp.

[safern]

@ericstj what do you think about harvesting the netstandard, ns1.1 and uap10.0.16299 implementations for System.Buffers? That would freeze the NS2.0 API surface area. I think we should only harvest downlevel (ns1.1 and uap16299) and for NS2.0 use the shared sources. Delete DefaultArrayPool and just use ConfigurableArrayPool.

[ericstj]

NS2 surface area is already frozen since it was inbox in NCA2.0 and UAP. I'm ok harvesting. Same caveats apply as elsewhere, if we need to service the old impls we'll need to undo harvesting or do it in a servicing branch but we can cross that bridge when we get there.

[safern]

Oh ok makes sense. Then it makes sense to harvest the impl for NS2 and downlevel.

I've renamed the issue and self assigned.