PR Feedback

* DefaultObjectPool left unchanged (expect access modifiers) + manual inlining ob scan-methods
* DisposableObjectPool made internal and it handles Return self, thus keeping the DefaultObjectPool fast
\n\nCommit migrated from a7dc496507

src/ObjectPool/src/DefaultObjectPool.cs

@@ -10,13 +10,13 @@ namespace Microsoft.Extensions.ObjectPool
 {
     public class DefaultObjectPool<T> : ObjectPool<T> where T : class
     {
-        protected readonly ObjectWrapper[] _items;
-        private readonly IPooledObjectPolicy<T> _policy;
-        private readonly bool _isDefaultPolicy;
-        protected T _firstItem;
+        private protected readonly ObjectWrapper[] _items;
+        private protected readonly IPooledObjectPolicy<T> _policy;
+        private protected readonly bool _isDefaultPolicy;
+        private protected T _firstItem;
 
         // This class was introduced in 2.1 to avoid the interface call where possible
-        private readonly PooledObjectPolicy<T> _fastPolicy;
+        private protected readonly PooledObjectPolicy<T> _fastPolicy;
 
         public DefaultObjectPool(IPooledObjectPolicy<T> policy)
             : this(policy, Environment.ProcessorCount * 2)
@@ -45,69 +45,43 @@ namespace Microsoft.Extensions.ObjectPool
             var item = _firstItem;
             if (item == null || Interlocked.CompareExchange(ref _firstItem, null, item) != item)
             {
-                item = GetViaScan();
+                var items = _items;
+                for (var i = 0; i < items.Length; i++)
+                {
+                    item = items[i].Element;
+                    if (item != null && Interlocked.CompareExchange(ref items[i].Element, null, item) == item)
+                    {
+                        return item;
+                    }
+                }
+
+                item = Create();
             }
 
             return item;
         }
 
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private T GetViaScan()
-        {
-            var items = _items;
-            for (var i = 0; i < items.Length; i++)
-            {
-                var item = items[i].Element;
-                if (item != null && Interlocked.CompareExchange(ref items[i].Element, null, item) == item)
-                {
-                    return item;
-                }
-            }
-
-            return Create();
-        }
-
         // Non-inline to improve its code quality as uncommon path
         [MethodImpl(MethodImplOptions.NoInlining)]
         private T Create() => _fastPolicy?.Create() ?? _policy.Create();
 
-        public override void Return(T obj) => ReturnCore(obj);
-
-        protected bool ReturnCore(T obj)
+        public override void Return(T obj)
         {
             if (_isDefaultPolicy || (_fastPolicy?.Return(obj) ?? _policy.Return(obj)))
             {
-                if (_firstItem == null && Interlocked.CompareExchange(ref _firstItem, obj, null) == null)
+                if (_firstItem != null || Interlocked.CompareExchange(ref _firstItem, obj, null) != null)
                 {
-                    return true;        // returned to pool
-                }
-                else
-                {
-                    return ReturnViaScan(obj);
+                    var items = _items;
+                    for (var i = 0; i < items.Length && Interlocked.CompareExchange(ref items[i].Element, obj, null) != null; ++i)
+                    {
+                    }
                 }
             }
-
-            return false;           // not returned to pool
-        }
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private bool ReturnViaScan(T obj)
-        {
-            var items = _items;
-            for (var i = 0; i < items.Length; i++)
-            {
-                if (Interlocked.CompareExchange(ref items[i].Element, obj, null) == null)
-                {
-                    return true;
-                }
-            }
-
-            return false;
         }
 
         // PERF: the struct wrapper avoids array-covariance-checks from the runtime when assigning to elements of the array.
         [DebuggerDisplay("{Element}")]
-        protected struct ObjectWrapper
+        private protected struct ObjectWrapper
         {
             public T Element;
         }

src/ObjectPool/src/DefaultObjectPoolProvider.cs

@@ -11,6 +11,11 @@ namespace Microsoft.Extensions.ObjectPool
 
         public override ObjectPool<T> Create<T>(IPooledObjectPolicy<T> policy)
         {
+            if (policy == null)
+            {
+                throw new ArgumentNullException(nameof(policy));
+            }
+
             if (typeof(IDisposable).IsAssignableFrom(typeof(T)))
             {
                 return new DisposableObjectPool<T>(policy, MaximumRetained);

src/ObjectPool/src/DisposableObjectPool.cs

@@ -2,11 +2,12 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Runtime.CompilerServices;
 using System.Threading;
 
 namespace Microsoft.Extensions.ObjectPool
 {
-    public class DisposableObjectPool<T> : DefaultObjectPool<T>, IDisposable where T : class
+    internal sealed class DisposableObjectPool<T> : DefaultObjectPool<T>, IDisposable where T : class
     {
         private volatile bool _isDisposed;
 
@@ -31,7 +32,7 @@ namespace Microsoft.Extensions.ObjectPool
 
             void ThrowObjectDisposedException()
             {
-                throw new ObjectDisposedException(this.GetType().Name);
+                throw new ObjectDisposedException(GetType().Name);
             }
         }
 
@@ -44,8 +45,32 @@ namespace Microsoft.Extensions.ObjectPool
             }
         }
 
+        private bool ReturnCore(T obj)
+        {
+            bool returnedTooPool = false;
+
+            if (_isDefaultPolicy || (_fastPolicy?.Return(obj) ?? _policy.Return(obj)))
+            {
+                if (_firstItem == null && Interlocked.CompareExchange(ref _firstItem, obj, null) == null)
+                {
+                    returnedTooPool = true;
+                }
+                else
+                {
+                    var items = _items;
+                    for (var i = 0; i < items.Length && !(returnedTooPool = Interlocked.CompareExchange(ref items[i].Element, obj, null) == null); i++)
+                    {
+                    }
+                }
+            }
+
+            return returnedTooPool;
+        }
+
         public void Dispose()
         {
+            _isDisposed = true;
+
             DisposeItem(_firstItem);
             _firstItem = null;
 
@@ -55,8 +80,6 @@ namespace Microsoft.Extensions.ObjectPool
                 DisposeItem(items[i].Element);
                 items[i].Element = null;
             }
-
-            _isDisposed = true;
         }
 
         private void DisposeItem(T item)

src/ObjectPool/src/Properties/AssemblyInfo.cs

@@ -0,0 +1,3 @@
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("Microsoft.Extensions.ObjectPool.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]

src/ObjectPool/test/DisposableObjectPoolTest.cs

@@ -2,12 +2,61 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Generic;
 using Xunit;
 
 namespace Microsoft.Extensions.ObjectPool
 {
     public class DisposableObjectPoolTest
     {
+        [Fact]
+        public void DisposableObjectPoolWithDefaultPolicy_GetAnd_ReturnObject_SameInstance()
+        {
+            // Arrange
+            var pool = new DisposableObjectPool<object>(new DefaultPooledObjectPolicy<object>());
+
+            var obj1 = pool.Get();
+            pool.Return(obj1);
+
+            // Act
+            var obj2 = pool.Get();
+
+            // Assert
+            Assert.Same(obj1, obj2);
+        }
+
+        [Fact]
+        public void DisposableObjectPool_GetAndReturnObject_SameInstance()
+        {
+            // Arrange
+            var pool = new DisposableObjectPool<List<int>>(new ListPolicy());
+
+            var list1 = pool.Get();
+            pool.Return(list1);
+
+            // Act
+            var list2 = pool.Get();
+
+            // Assert
+            Assert.Same(list1, list2);
+        }
+
+        [Fact]
+        public void DisposableObjectPool_Return_RejectedByPolicy()
+        {
+            // Arrange
+            var pool = new DisposableObjectPool<List<int>>(new ListPolicy());
+            var list1 = pool.Get();
+            list1.Capacity = 20;
+
+            // Act
+            pool.Return(list1);
+            var list2 = pool.Get();
+
+            // Assert
+            Assert.NotSame(list1, list2);
+        }
+
         [Fact]
         public void DisposableObjectPoolWithOneElement_Dispose_ObjectDisposed()
         {
@@ -73,6 +122,19 @@ namespace Microsoft.Extensions.ObjectPool
             Assert.True(obj.IsDisposed);
         }
 
+        private class ListPolicy : IPooledObjectPolicy<List<int>>
+        {
+            public List<int> Create()
+            {
+                return new List<int>(17);
+            }
+
+            public bool Return(List<int> obj)
+            {
+                return obj.Capacity == 17;
+            }
+        }
+
         private class DisposableObject : IDisposable
         {
             public bool IsDisposed { get; private set; }

src/ObjectPool/test/ThreadingTest.cs

@@ -13,10 +13,22 @@ namespace Microsoft.Extensions.ObjectPool
         private bool _foundError;
 
         [Fact]
-        public void RunThreadingTest()
+        public void DefaultObjectPool_RunThreadingTest()
+        {
+            _pool = new DefaultObjectPool<Item>(new DefaultPooledObjectPolicy<Item>(), 10);
+            RunThreadingTest();
+        }
+
+        [Fact]
+        public void DisposableObjectPool_RunThreadingTest()
+        {
+            _pool = new DisposableObjectPool<Item>(new DefaultPooledObjectPolicy<Item>(), 10);
+            RunThreadingTest();
+        }
+
+        private void RunThreadingTest()
         {
             _cts = new CancellationTokenSource();
-            _pool = new DefaultObjectPool<Item>(new DefaultPooledObjectPolicy<Item>(), 10);
 
             var threads = new Thread[8];
             for (var i = 0; i < threads.Length; i++)
@@ -66,7 +78,7 @@ namespace Microsoft.Extensions.ObjectPool
                 _pool.Return(obj2);
             }
         }
-        
+
         private class Item
         {
             public int i = 0;