PooledList.Dispose semantics

[dzmitry-lahoda]

As I see current semantics is to return array into pool, set capacity-size to 0, and allow use later.

        /// <summary>
        /// Returns the internal buffers to the ArrayPool.
        /// </summary>
        public void Dispose()
        {
            ReturnArray();
            _size = 0;
            _version++;
        }

This contradicts with corefx classes Dispose when you cannot use object after Dispose.

Real semantic seems to be Dispose = Reallocate(capacity: 0). ReturnPool => Reallocate(capacity: 0). I suggest to rename Dispose to something else.

Or document:

        /// <summary>
        /// Returns the internal buffers to the ArrayPool and sets capacity to zero. Instance still can be used.
        /// </summary>

[jtmueller]

Renaming the method to something other that Dispose is a non-starter, because the whole point of this is to be able to use the object with a using statement, and have the underlying array automatically returned to the pool at the end of that block of code. The IDisposable interface requires the name Dispose, and explicit interface implementation plus renaming the public Dispose method would break the published API of these objects and add confusion for people using them.

I could set a boolean flag when you call Dispose, and then check that flag in each and every method and property so I can throw an ObjectDisposedException if you try to use the object after disposing it. I just don't see the point of it. That's a whole lot of tedious work that also inserts at least two IL operations into each method and property, for no tangible benefit.

Anyone who is familiar with the IDisposable pattern is aware that you shouldn't count on being able to use an object after disposing it. Just because technically you can use the object after disposing it in this case doesn't change the general rule. Where is the actual harm done by the fact that doing something you're not supposed to in the general case doesn't actually break things in this case?

So, we can hurt performance of operations in tight loops by checking a class-level variable on every operation, and in exchange we get.. I'm not really sure. I'm afraid I don't see a compelling reason to make any change in this area.

[dzmitry-lahoda]

Use case is next.

The is hundred pooled lists. There is a timer which beats 60 times per second. Each time, code sparsely and randomly initializes some lists with one to thousand elements. When list is used, it can be Clear. But clear still retains memory from pool. So total memory used and memory per list (current rented array) is bigger than if to call Dispose directly each time. Knowing that after Dispose list could be reused. So dispose somewhat changes meaning...

[jtmueller]

Okay, so I will grant that the fact that a PooledList is technically still usable after calling Dispose is an implementation detail that would not be obvious to someone who had not read the source code.

In your scenario, someone who assumes that they can't re-use a list after calling Dispose still has options, though. That person could call list.Clear();, followed by list.Capacity = 0;. This is basically the equivalent of calling Dispose, and will return the rented array to the the array pool. In fact, you wouldn't even need to call Clear because just setting the capacity to zero will take care of returning the array to the pool and settinglist.Count to zero.

Since you can get the same memory characteristics as in your Dispose scenario by using the Capacity property, even if you don't know pooled lists are still usable after Dispose, would it be acceptable to leave Dispose as-is, but add some additional documentation to the Capacity property to let the user know what happens when it's set to zero?

[dzmitry-lahoda]

ArgumentOutOfRangeException
Capacity is set to a value that is less than Count.

I guess better to retain semantics of Capacity setter of List(no checked code of PooledList if semantics already changed now). Dispose may be retained to possible usage when list cannot be used after dispose, if ever case appears, if. Other options to have Count setter ('List` has only getter) which returns to pool, but unclear if case of partial return is real.

So, I will check Clear (in case of ctor parameter do not clear data on free, clear should not clear either) and Capacity = 0 if it works as good as Dispose(same code executed). And name it in extensions method like ReturnArray or like (seems nor Return nor Free should not be used as is).

[jtmueller]

Ah, good point. Capacity by itself wouldn't work, but Clear, then Capacity should do the trick.

[dzmitry-lahoda]

I have extension ClearCapacity() which works.