5
\$\begingroup\$

I have written my own object pool class called ObjectPool<T> which uses SpinLock and Stack<T>:

public class ObjectPool<T> where T : class
{
    private int _size;
    private Func<T> _factory;
    private SpinLock _spinLock;

    // storage for the pool objects.
    // the first item is expected to be most often case.
    private T _firstItem;
    private Stack<T> _cache; //Use Queue<T> for first-in-first-out (FIFO)

    public ObjectPool(Func<T> factory) : this(factory, Environment.ProcessorCount * 2) { }

    public ObjectPool(Func<T> factory, int size)
    {
        _factory = factory;
        _size = size;
        _cache = new Stack<T>();
        _spinLock = new SpinLock(false);
    }

    /// <summary>
    /// Produces an instance.
    /// </summary>
    public T Rent()
    {
        bool lockTaken = false;
        _spinLock.Enter(ref lockTaken);

        try
        {
            T instance = _firstItem;
            if (instance == null || instance != Interlocked.CompareExchange(ref _firstItem, null, instance))
            {
                instance = RentFromCache();
            }

            return instance;
        }
        finally
        {
            if (lockTaken)
            {
                _spinLock.Exit(false);
            }
        }
    }

    private T RentFromCache()
    {
        var cache = _cache;

        if (cache.Count > 0)
        {
            T instance = _cache.Pop();

            if (instance != null)
            {
                return instance;
            }
        }

        return CreateInstance();
    }

    private T CreateInstance()
    {
        var instance = _factory();
        return instance;
    }

    /// <summary>
    /// Returns objects to the pool.
    /// </summary>
    public void Return(T obj)
    {
        bool lockTaken = false;
        _spinLock.Enter(ref lockTaken);

        try
        {
            if (_firstItem == null)
            {
                // worst case scenario: two objects may be stored into same slot.
                _firstItem = obj;
            }
            else
            {
                ReturnToCache(obj);
            }
        }
        finally
        {
            if (lockTaken)
            {
                _spinLock.Exit(false);
            }
        }
    }

    private void ReturnToCache(T obj)
    {
        var cache = _cache;

        if (cache.Count >= _size)
        {
            // not a big fan of doing it this way
            return;
        }

        // worst case scenario: two objects may be stored into same slot.
        cache.Push(obj);
    }
}

I'm currently using this object pool to get high performance out of a Socket by making a pool of asynchronous socket operations (ObjectPool<SocketAsyncEventArgs>).

Note: I'm receiving UDP packets, which might help illustrate why some quirks exist (that won't work well for some generic-cases) in this object pool's design, such as using a LIFO pool.

It's working great so far. However, I'd appreciate some expert code review. I am hoping for advice on where it can be improved, further optimized, or if I have missed something important.


Additional Questions:

  • I'm wondering if ConcurrentStack<T> would provide me with any added benefits?
\$\endgroup\$
3
  • \$\begingroup\$ Have you checked if the pool is really faster than just creating new objects an let the garbage collector clean them up? \$\endgroup\$
    – JanDotNet
    Commented Feb 27, 2017 at 7:13
  • \$\begingroup\$ @JanDotNet If we just used the _firstItem field as the "fastest" case, it performs considerably faster than to "new-up" an object and dispose it. With socket operations -- naturally, pooling these will save allocation and deallocation costs significantly. \$\endgroup\$
    – Svek
    Commented Feb 27, 2017 at 15:03
  • 2
    \$\begingroup\$ Using a ConcurrentStack will simplify your code a bit but only testing will determine if it's faster. You would get rid of the spinlock and have it handle that for you. \$\endgroup\$ Commented Feb 27, 2017 at 15:13

2 Answers 2

4
\$\begingroup\$

Because you don't change

private int _size;
private Func<T> _factory;
private Stack<T> _cache;

private SpinLock _spinLock; //Use Queue<T> for first-in-first-out (FIFO)

you should make them readonly.

Correction after the comment of t3chb0t:

Don't make the SpinLock readonly. Further clarification: https://stackoverflow.com/a/9235028/2655508

The underlying problem is that the C# compiler creates a copy of a readonly value type field when you call a non-static method on it and executes that method on the copy - because the method could have side effects that change the value of the struct - which is not allowed for readonly fields.


private T RentFromCache()
{
    var cache = _cache;

    if (cache.Count > 0)
    {
        T instance = _cache.Pop();

        if (instance != null)
        {
            return instance;
        }
    }

    return CreateInstance();
}  

Here you are using var cache only for checking its Count property. I don't see a reason to not use the class level _cache directly. Using the class level variable directly won't confuse the reader of the code. The same applies to ReturnToCache() as well.


\$\endgroup\$
1
  • \$\begingroup\$ oh! Actually, for your second code block, my code was supposed to be cache.Pop(); not _cache.Pop();. Thank you for pointing that out to me. \$\endgroup\$
    – Svek
    Commented Feb 27, 2017 at 14:17
2
\$\begingroup\$

I'd like to add my 5 cents:

  1. First, I think you should carefully consider whether you actually need those optimizations. Debugging and maintaining multi-threaded code is a lot of work, and you don't have to make it harder unless there is a good reason for it. If you need a pool, does it have to use SpinLock and Interlocked.CompareExchange? Is simple lock too slow? Is ConcurrentStack too slow? You should start by trying the simplest option first. Only when you have objective proof that simple options do not meet your performance requirements you should consider more complex solutions. It looks like you are doing it the other way around.
  2. Potential design issue: in order to release a pooled object I have to have a reference to the pool it came from. In some cases this reference might not be that easy to get.
  3. Potential design issue: your pool does not count references. If ClassA and ClassB ever get a reference to the same pooled object, it is going to be hard for ClassA to release it without risking that it might still be in use in ClassB.
\$\endgroup\$
3
  • \$\begingroup\$ Thank you for your review! Originally, I had set out to maximize performance by taking on the hardships of manual memory / thread management. Regarding your point about locking, I had added the question about ConcurrentStack after I had submitted / written the code -- honestly, I had plain forgotten about it when I wrote the object pool class... I guess the only way to know if it would be an enhancement would be to run additional lab tests. At the moment, this object pool is humming along quite nicely for our asynchronous socket operations. \$\endgroup\$
    – Svek
    Commented Feb 28, 2017 at 13:37
  • \$\begingroup\$ To your second point, I'm not clear as to what you mean. If we are referencing the object pool in a process, in what scenarios would a process that uses the object pool not be able to get reference to it? Could you help me understand this? Maybe it's beyond the context of my use case, so I'm not able to see it in the context you are thinking of... \$\endgroup\$
    – Svek
    Commented Feb 28, 2017 at 13:45
  • 1
    \$\begingroup\$ @Svek, yes, obviously you are referencing a pool somewhere. The point it that as soon as you start passing pooled-objects between your classes, you gonna need to pass pool reference as well, in order for those classes to release pooled-objects. This will result in new dependency on pool in classes, that might otherwise not need it. But then again, you do not show your actual use case, so it might not be a problem for you. \$\endgroup\$
    – Nikita B
    Commented Mar 1, 2017 at 8:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.