From 5e19936a9f1a1e6d46878753bc7a8ca14001e125 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCnther=20Foidl?= Date: Sun, 20 Jan 2019 21:01:59 +0100 Subject: [PATCH] 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 https://github.com/dotnet/extensions/commit/a7dc496507e10fbf56e4b76a156b4d0a0f22bcba --- src/ObjectPool/src/DefaultObjectPool.cs | 72 ++++++------------- .../src/DefaultObjectPoolProvider.cs | 5 ++ src/ObjectPool/src/DisposableObjectPool.cs | 31 ++++++-- src/ObjectPool/src/Properties/AssemblyInfo.cs | 3 + .../test/DisposableObjectPoolTest.cs | 62 ++++++++++++++++ src/ObjectPool/test/ThreadingTest.cs | 18 ++++- 6 files changed, 135 insertions(+), 56 deletions(-) create mode 100644 src/ObjectPool/src/Properties/AssemblyInfo.cs diff --git a/src/ObjectPool/src/DefaultObjectPool.cs b/src/ObjectPool/src/DefaultObjectPool.cs index 6ecc923140..070508a014 100644 --- a/src/ObjectPool/src/DefaultObjectPool.cs +++ b/src/ObjectPool/src/DefaultObjectPool.cs @@ -10,13 +10,13 @@ namespace Microsoft.Extensions.ObjectPool { public class DefaultObjectPool : ObjectPool where T : class { - protected readonly ObjectWrapper[] _items; - private readonly IPooledObjectPolicy _policy; - private readonly bool _isDefaultPolicy; - protected T _firstItem; + private protected readonly ObjectWrapper[] _items; + private protected readonly IPooledObjectPolicy _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 _fastPolicy; + private protected readonly PooledObjectPolicy _fastPolicy; public DefaultObjectPool(IPooledObjectPolicy 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; } diff --git a/src/ObjectPool/src/DefaultObjectPoolProvider.cs b/src/ObjectPool/src/DefaultObjectPoolProvider.cs index cedacaedbd..2e7767ab35 100644 --- a/src/ObjectPool/src/DefaultObjectPoolProvider.cs +++ b/src/ObjectPool/src/DefaultObjectPoolProvider.cs @@ -11,6 +11,11 @@ namespace Microsoft.Extensions.ObjectPool public override ObjectPool Create(IPooledObjectPolicy policy) { + if (policy == null) + { + throw new ArgumentNullException(nameof(policy)); + } + if (typeof(IDisposable).IsAssignableFrom(typeof(T))) { return new DisposableObjectPool(policy, MaximumRetained); diff --git a/src/ObjectPool/src/DisposableObjectPool.cs b/src/ObjectPool/src/DisposableObjectPool.cs index ca9288a6d6..17ada443e5 100644 --- a/src/ObjectPool/src/DisposableObjectPool.cs +++ b/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 : DefaultObjectPool, IDisposable where T : class + internal sealed class DisposableObjectPool : DefaultObjectPool, 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) diff --git a/src/ObjectPool/src/Properties/AssemblyInfo.cs b/src/ObjectPool/src/Properties/AssemblyInfo.cs new file mode 100644 index 0000000000..121f8990b1 --- /dev/null +++ b/src/ObjectPool/src/Properties/AssemblyInfo.cs @@ -0,0 +1,3 @@ +using System.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("Microsoft.Extensions.ObjectPool.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] diff --git a/src/ObjectPool/test/DisposableObjectPoolTest.cs b/src/ObjectPool/test/DisposableObjectPoolTest.cs index 4bc9142efb..3bcefbaf66 100644 --- a/src/ObjectPool/test/DisposableObjectPoolTest.cs +++ b/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(new DefaultPooledObjectPolicy()); + + 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>(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>(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> + { + public List Create() + { + return new List(17); + } + + public bool Return(List obj) + { + return obj.Capacity == 17; + } + } + private class DisposableObject : IDisposable { public bool IsDisposed { get; private set; } diff --git a/src/ObjectPool/test/ThreadingTest.cs b/src/ObjectPool/test/ThreadingTest.cs index 541bc5ffd4..dbab7a5301 100644 --- a/src/ObjectPool/test/ThreadingTest.cs +++ b/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(new DefaultPooledObjectPolicy(), 10); + RunThreadingTest(); + } + + [Fact] + public void DisposableObjectPool_RunThreadingTest() + { + _pool = new DisposableObjectPool(new DefaultPooledObjectPolicy(), 10); + RunThreadingTest(); + } + + private void RunThreadingTest() { _cts = new CancellationTokenSource(); - _pool = new DefaultObjectPool(new DefaultPooledObjectPolicy(), 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;