Add MemoryPool apis #26563

ghost · 2018-01-24T16:05:07Z

Fixes https://github.com/dotnet/corefx/issues/26357

stephentoub · 2018-01-24T17:06:49Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+
+            public sealed override bool Release()
+            {
+                int newRefCount = Interlocked.Decrement(ref _referenceCount);


What's the goal of the interlockeds being used here? As it stands, for example, even if Release throws, the _referenceCount will already have gone negative, and code could subsequently continue to use the object that now has a negative reference count... so then, for example, another thread could call Retain, have it succeed, but the count could still be <= 0 and IsRetained would return false.

Seems like either we shouldn't care about thread-safety, in which case we don't need to use interlockeds, or we should care, in which case this should be more robust and use a standard Interlocked.CompareExchange loop for these kinds of operations.

@ahsonkhan, @KrzysztofCwalina - what are the thread-safety goals here? These interlocked calls were all part of the original prototype in corefxlab.

The main goal of the implementation is to detect use after free. "Detect" is the key word here. I dont think we can make it 100% thread safe without unacceptable perf hit. For that to happen, we would need to do what @stephentoub says plus synchronize access to the array (the Span property, for example).

BTW, I don't mind making this method more robust, I am just not sure it will add value without also changing get_Span, which I don't think we can change without significant perf hit.

Ok - I'm inclined to drop any pretense of thread-safety then. It's pretty well established the .NET objects are thread-unsafe in the 90% case and synchronization is outsourced to the users.

Wait, gat_Span is synchronized as it checks for IsDisposed first. Maybe we are not too far from having valuable "more safety". Still there is an issue of Memory callers not having to call Retain, but at least I think we can make it totally safe for those who want to religiously use Retain/Release.

It's not a thread-safe check though - when I see "synchronized", I think thread-safe.

stephentoub · 2018-01-24T17:07:45Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+            }
+
+            public sealed override bool Release()
+            {


Retain checks IsDisposed. Release doesn't need to? And we don't need disposal to be coordinated with the interlockeds used by Retain/Release? (Goes back to my question about what the thread-safety goals are.)

@ahsonkhan khan, @KrzysztofCwalina - care to address this? I noticed this asymmetry in the corefxlab code too but figured there may be a reason (perhaps because if someone didn't follow the normal dispose pattern, Release can happen as part of a MemoryHandle finalization which could happen after the OwnedMemory it came from got disposed - and we don't want an exception being thrown there?)

If somebody calls "release" they simply communicate that they wont be using this OwnedMemory (OM) anymore. I am not sure why we would want to throw on already disposed objects. It does not seem to be adding to safety of the system and just makes the Release callsite subject to exceptions. Having said that, I don't mind changing it, if we feel it adds value.

If you are calling Release() on already disposed object, it means that you have use-after-free security bug. The calls to Rent and Release have to be always matched.

The C/C++ memory allocators fail fast right away and abort the program when they detect use after free. We may want to do the same instead of throwing catchable exception.

stephentoub · 2018-01-24T17:08:46Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+
+            protected sealed override void Dispose(bool disposing)
+            {
+                T[] array = Interlocked.Exchange(ref _array, null);


What if it's currently retained?

Then it throws an InvalidOperationException. That check is encapsulated in the OwnedMemory class itself.

What throws an InvalidOperationException? If you Dispose of this while it's already retained, then the array is likely already in use, but we're returning it to the pool here.

Right here:

corefx/src/Common/src/CoreLib/System/Buffers/OwnedMemory.cs

Line 63 in 83f35ef

ThrowHelper.ThrowInvalidOperationException_OutstandingReferences();

But you're not calling base.Dispose(disposing). So who calls that?

Oh, sorry, different overload. I see.

stephentoub · 2018-01-24T17:09:14Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+        {
+            if (minimumBufferSize == -1)
+                minimumBufferSize = DefaultSize;
+            else if (minimumBufferSize > MaxBufferSize || minimumBufferSize < 1)


Does MaxBufferSize get devirtualized? Otherwise, this is another virtual call.

I would hope the JIT can do that considering the class is sealed at this level. Though it's easy enough to substitute a const to be safe.

Why not allow minimumBufferSize == 0?

stephentoub · 2018-01-24T17:10:39Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+            else if (minimumBufferSize > MaxBufferSize || minimumBufferSize < 1)
+                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.minimumBufferSize);
+
+            return new ArrayMemoryPoolBuffer(minimumBufferSize);


Every rental allocates?

It's not clear to me how that could be avoided. The OwnedMemory wrapper object is meant to be used in a "using () {}" type-pattern. If we tried to pool those as well, we'd be handing out IDisposable objects that the caller isn't meant to dispose.

I'm not really commenting on the implementation so much as the MemoryPool design. Seems problematic if the default implementation that most folks will use allocates on every Rent.

This depends on what the MemoryPool abstraction is meant to be used for.

I see it designed for large buffers. You get large buffer and allocate a little bit in the process. I do not think that allocation is problematic in this scenario.

The default size here is 4096, which is also a common size used with ArrayPool, and for ArrayPool, there have been complaints about things as small as the virtual dispatch cost associated with calling ArrayPool.Shared.Rent/Release; even a small allocation and the associated GC impact is at least as much as that. I'm skeptical this will only be used for large buffers; of course, if we add costs like such allocations on every Rent/Release, we may certainly force it in that direction.

If you want a bare bone malloc/free, you should use ArrayPool. MemoryPool is higher level abstraction.

And yes - we need to fix ArrayPool to not have the unnecessary overheads and to not leak.

Still seems like it's going to be a problem. I guess we'll have to agree to disagree.

The memory pool in kestrel pools the OwnedMemory<byte> objects 😄

You can certainly pool OwnedMemory memory if you know what you are doing. It increases your security risk profile.

We used to have a cookie in OM to allow it to be pooled safely. We removed the cookie during the performance push, but I am not sure it actually helps performance, yet it makes it so much more difficult to pool OM safely.

jkotas · 2018-01-24T17:25:15Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+    {
+        private const int DefaultSize = 4096;
+
+        private static readonly ArrayMemoryPool<T> s_shared = new ArrayMemoryPool<T>();


This is unnecessary extra layer MemoryPool<T> Shared can allocate ArrayMemoryPool directly.

Yeah, I think we should use the shared array pool directly.

jkotas · 2018-01-24T17:25:51Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+        private static readonly ArrayMemoryPool<T> s_shared = new ArrayMemoryPool<T>();
+        public static new ArrayMemoryPool<T> Shared => s_shared;
+
+        public sealed override int MaxBufferSize => 1024 * 1024 * 1024;


Why is this 1GB and not 2GB?

Yeah, I think we should change it to 2GB

jkotas · 2018-01-24T17:28:19Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+{
+    internal sealed partial class ArrayMemoryPool<T> : MemoryPool<T>
+    {
+        private const int DefaultSize = 4096;


Is this a good default size even for large Ts ? Should this rather be 1 + (4095 / sizeof(T)) ?

KrzysztofCwalina · 2018-01-24T17:29:22Z

src/System.Memory/ref/System.Memory.cs

+    public abstract class MemoryPool<T> : IDisposable
+    {
+        public static System.Buffers.MemoryPool<T> Shared { get; }
+        public abstract System.Buffers.OwnedMemory<T> Rent(int minBufferSize=-1);


We had a discussion at the API review about the default here. Did we really settle on -1? Why not have the default be 0?

Why not have the default be 0?

FWIW, ArrayPool.Shared.Rent(0) gives back an empty array. Then again, it also throws for -1.

IIRC, we have settled on -1.

It's on youtube, it was -1.

you mean "it's on the internet"? :-)

jkotas · 2018-01-24T17:30:16Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+        private sealed class ArrayMemoryPoolBuffer : OwnedMemory<T>
+        {
+            private T[] _array;
+            private bool _disposed;


Can we use array == null to signal disposes instead of an extra bool flag?

Seems reasonable.

KrzysztofCwalina · 2018-01-24T18:33:33Z

src/System.Memory/tests/MemoryPool/MemoryPool.cs

+        {
+            get
+            {
+                yield return new object[] { 0 };


Why is 0 bad? ArrayPool allows to rent empty buffers.

Another artifact from the corefxlab code. Since we've settled on -1 as the sentinel, I'll remove that restriction.

KrzysztofCwalina · 2018-01-24T18:35:59Z

src/System.Memory/tests/MemoryPool/MemoryPool.cs

+        }
+
+        [Fact]
+        public static void RefCounting()


Could you add some stress tests too to ensure we don't let people access the array (the Span) after Dispose on a separate thread. Also, to ensure that Dispose throws is there are outstanding Retain (called on a different thread).

Dispose on a separate thread

That would be contingent on whether we want to guarantee thread-safety. I'm inclined against that.

On a separate note, is CI really the place to be doing stress tests? CI should be about prevent clearly bad stuff from going in, not randomly holding up PR's because of intermittent stress failures.

The Dispose throwing on an outstanding ref-count is already tested in the OwnedMemory tests (and the OwnedMemory class is the one that does that check so checking for individual variants doesn't add any code coverage.) It's an invariant of OwnedMemory objects in general and not specific to MemoryPool.

ahsonkhan · 2018-01-25T03:04:38Z

src/System.Memory/src/System.Memory.csproj

@@ -33,6 +33,9 @@
    <Compile Include="System\Buffers\Binary\Writer.cs" />
    <Compile Include="System\Buffers\Binary\WriterBigEndian.cs" />
    <Compile Include="System\Buffers\Binary\WriterLittleEndian.cs" />
+    <Compile Include="System\Buffers\MemoryPool.cs" />
+    <Compile Include="System\Buffers\ArrayMemoryPool.cs" />
+    <Compile Include="System\Buffers\ArrayMemoryPool.ArrayMemoryPoolBuffer.cs" />


nit: fix sort order. put it above
<Compile Include="System\Buffers\OperationStatus.cs" />

Also, the file name ArrayMemoryPool.ArrayMemoryPoolBuffer.cs is confusing. Do we have this type of naming based on nested classes elsewhere?

The double-dot notation is used generally when partial classes are used. This is just another instance of that.

ahsonkhan · 2018-01-25T03:07:35Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+                _array = ArrayPool<T>.Shared.Rent(size);
+            }
+
+            public sealed override int Length => _array.Length;


Do properties and methods on a sealed class need to marked as sealed as well?

Do properties and methods on a sealed class need to marked as sealed as well?

No

ahsonkhan · 2018-01-25T03:10:19Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+                unsafe
+                {
+                    Retain(); // this checks IsDisposed
+                    GCHandle handle = GCHandle.Alloc(_array, GCHandleType.Pinned);


Add argument checks for byteOffset?

Similar to: https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/Memory/CustomMemoryForTest.cs#L47

if (byteOffset < 0 || (byteOffset/Unsafe.SizeOf<T>()) > _array.Length) throw

This check isn't right - for an array of 10 ints, "40" should be a valid input, but not 41, 42 and 43.

This seems like low-level api to be doing this kind of check - I'll put in but it gets tricky since MemoryPool owners don't have strict control over the sizes they get back. The byteOffset might not actually fit in an Int32.

ahsonkhan · 2018-01-25T03:15:55Z

src/System.Memory/src/System/Buffers/MemoryPool.cs

+        public static MemoryPool<T> Shared => ArrayMemoryPool<T>.Shared;
+
+        /// <summary>
+        /// Returns a memory block capable of holding at least <paramref name="minBufferSize">minBufferSize</paramref> elements of T.


Returns a memory block capable of holding at least <paramref name="minBufferSize"/> elements of T.?

ahsonkhan · 2018-01-25T03:17:36Z

src/System.Memory/src/System/Buffers/MemoryPool.cs

+        /// <summary>
+        /// Returns a memory block capable of holding at least <paramref name="minBufferSize">minBufferSize</paramref> elements of T.
+        /// </summary>
+        /// <param name="minBufferSize">The minimum required size of the buffer in elements. If -1 is passed, this is set to a default value for the pool.</param>


nit: This is awkward to read. Do we need to mention in elements?

I'll just nuke that part of the phrase - the summary plus the param name makes it obvious.

ahsonkhan · 2018-01-25T03:18:00Z

src/System.Memory/src/System/Buffers/MemoryPool.cs

+        /// Returns a memory block capable of holding at least <paramref name="minBufferSize">minBufferSize</paramref> elements of T.
+        /// </summary>
+        /// <param name="minBufferSize">The minimum required size of the buffer in elements. If -1 is passed, this is set to a default value for the pool.</param>
+        /// <returns></returns>


Remove empty returns tag.

ahsonkhan · 2018-01-25T03:24:50Z

src/System.Memory/src/System/ThrowHelper.cs

        internal static void ThrowInvalidOperationException_OutstandingReferences() { throw CreateInvalidOperationException_OutstandingReferences(); }
        [MethodImpl(MethodImplOptions.NoInlining)]
        private static Exception CreateInvalidOperationException_OutstandingReferences() { return new InvalidOperationException(SR.OutstandingReferences); }

+        internal static void ThrowObjectDisposedException(string objectName) { throw CreateObjectDisposedException(objectName); }


We recently removed the objectName argument from ThrowObjectDisposedException_MemoryDisposed.
3f03814#diff-4021c6d7a5c4f084b4e8867d8cb0f973L67
@jkotas, why was that?

dotnet/coreclr#15909 (comment)

Thanks for the context! Should we keep passing the object name to the newly added ThrowObjectDisposedException method or remove that argument here as well?

ahsonkhan · 2018-01-25T03:31:21Z

src/System.Memory/tests/MemoryPool/MemoryPool.cs

+            OwnedMemory<int> block = MemoryPool<int>.Shared.Rent(42);
+            Assert.False(block.IsDisposed);
+            block.Dispose();
+            Assert.True(block.IsDisposed);


Should we add another block.Dispose() and check that block.IsDisposed remains true?

ahsonkhan · 2018-01-25T03:32:08Z

src/System.Memory/tests/MemoryPool/MemoryPool.cs

+        }
+
+        [Fact]
+        public static void NoPinAfterDispose()


How are we testing Pin after Dispose here?

stephentoub · 2018-01-25T17:06:01Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

+{
+    internal sealed partial class ArrayMemoryPool<T> : MemoryPool<T>
+    {
+        private const int s_maxBufferSize = int.MaxValue;


Nit: s_ prefix is used for statics; consts should be PascalCased.

I know - this is kinda tough case since the "best" name is already taken by the property. MaxBufferSizeConst just sounds awkward.

ahsonkhan · 2018-01-25T18:44:41Z

@atsushikan, can you please maintain the commit history during PR review? It makes it easier to view only the diffs between commits.

ghost · 2018-01-25T18:55:35Z

can you please maintain the commit history during PR review?

That's not so easy on GitHub when a PR stretches across days. I merge from upstream at least once a day so you either get a commit history that includes massive merges (and thus the main file list containing hundreds of unrelated changes, which someone complained about earlier) or a multi-commit rebase (which is treacherous and a pain in the neck and still results in a force push with new hashes so have to take on faith the "older" commits are actually the commits you reviewed previously.) Either way, there are drawbacks that make someone unhappy.

ahsonkhan · 2018-01-25T19:11:40Z

I merge from upstream at least once a day so you either get a commit history that includes massive merges (and thus the main file list containing hundreds of unrelated changes, which someone complained about earlier)

Why would merging from upstream result in unrelated changes to show up? Isn't the diff view against what is in master? So, we may see the Merge branch... commits in the PR, but the changes themselves shouldn't show up.

I would imagine it would be something like this (from #26232), which only shows my changes even though I have merged from upstream:

ghost · 2018-01-25T19:17:53Z

Why would merging from upstream result in unrelated changes to show up?

That was the experience about a year ago - people complained so I switched to the rebase-and-force-push method. Maybe GitHub is smarter now - I'd be happy to switch back (until someone complains again...)

ghost · 2018-01-25T23:05:23Z

@ahsonkhan - any further blocking issues?

jkotas · 2018-01-25T23:10:40Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

+                {
+                    Retain(); // this checks IsDisposed
+
+                    if (byteOffset != 0 && (((uint)byteOffset) - 1) / Unsafe.SizeOf<T>() >= _array.Length)


We should allow byteOffset that points right at the end of the array.

(See discussion at #25770 (comment))

It does allow that. We have a specific test for it: https://github.com/AtsushiKan/corefx/blob/mempool/src/System.Memory/tests/MemoryPool/MemoryPool.cs#L96

Yes, you are right.

ahsonkhan

LGTM.

Commit migrated from dotnet/corefx@a398684

ghost added the area-System.Memory label Jan 24, 2018

ghost self-assigned this Jan 24, 2018

ghost requested a review from ahsonkhan January 24, 2018 16:05

stephentoub reviewed Jan 24, 2018

View reviewed changes

jkotas reviewed Jan 24, 2018

View reviewed changes

KrzysztofCwalina reviewed Jan 24, 2018

View reviewed changes

jkotas reviewed Jan 24, 2018

View reviewed changes

KrzysztofCwalina reviewed Jan 24, 2018

View reviewed changes

ahsonkhan reviewed Jan 25, 2018

View reviewed changes

PR feedback

5390413

stephentoub reviewed Jan 25, 2018

View reviewed changes

ahsonkhan mentioned this pull request Jan 25, 2018

Fix Pin byteOffset bounds checks dotnet/corefxlab#2071

Merged

jkotas reviewed Jan 25, 2018

View reviewed changes

ahsonkhan approved these changes Jan 25, 2018

View reviewed changes

ghost merged commit a398684 into dotnet:master Jan 25, 2018

ahsonkhan mentioned this pull request Jan 26, 2018

System.Memory source cleanup and fix byteOffset check in tests #26598

Merged

ghost deleted the mempool branch January 26, 2018 15:11

ahsonkhan mentioned this pull request Jan 29, 2018

Remove MemoryPool and fix System.Buffers version dotnet/corefxlab#2086

Merged

karelz added this to the 2.1.0 milestone Feb 4, 2018

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

PR feedback (dotnet/corefx#26563)

e8ec66d

Commit migrated from dotnet/corefx@a398684

This pull request was closed.

Add MemoryPool apis #26563

Add MemoryPool apis #26563

Conversation

ghost commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jan 24, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan commented Jan 25, 2018

ghost commented Jan 25, 2018

ahsonkhan commented Jan 25, 2018 • edited Loading

ghost commented Jan 25, 2018

KrzysztofCwalina Jan 24, 2018 •

edited

Loading

ghost Jan 24, 2018 •

edited by ghost

Loading

jkotas Jan 24, 2018 •

edited

Loading

stephentoub Jan 24, 2018 •

edited

Loading

KrzysztofCwalina Jan 24, 2018 •

edited

Loading

jkotas Jan 24, 2018 •

edited

Loading

stephentoub Jan 24, 2018 •

edited

Loading

KrzysztofCwalina Jan 24, 2018 •

edited

Loading

KrzysztofCwalina Jan 24, 2018 •

edited

Loading

ahsonkhan Jan 25, 2018 •

edited

Loading

ahsonkhan commented Jan 25, 2018 •

edited

Loading