Reduce IEnumerator allocation via interface

[benaadams]

Revisiting #4505 /cc @stephentoub taking another shot at it...

If in addition to the struct enumerators another nested class was added to the generic collections that used the struct enumerator e.g. for List<T>

private class CachedIEnumerator : IEnumerator<T>
{
    private List<T> _list;
    private Enumerator _enumerator;

    internal CachedIEnumerator(List<T> list)
    {
        _list = list;
        _enumerator = new Enumerator(list);
    }
    internal CachedIEnumerator Reinitalize()
    {
        _enumerator = new Enumerator(_list);
        return this;
    }
    public bool MoveNext() => _enumerator.MoveNext();
    public T Current => _enumerator.Current;
    object IEnumerator.Current => Current;
    public void Dispose()
    {
        _enumerator = default(Enumerator);
        Interlocked.Exchange(ref _list._cachedIEnumerator, this);
    }
    public void Reset() => new NotSupportedException();
}

A new field added private CachedIEnumerator _cachedIEnumerator;

And the IEnumerable<T> implementations changed to

IEnumerator<T> IEnumerable<T>.GetEnumerator() {
    var enumerator = Interlocked.Exchange(ref _cachedIEnumerator, null);
    if (enumerator != null)
    {
        return enumerator.Reinitalize();
    }
    return new CachedIEnumerator(this);
}
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() {
    return ((IEnumerable<T>)this).GetEnumerator();
}

It would still allocate on first use, however foreach calls Dispose when its finished https://github.com/dotnet/coreclr/issues/1505 so this would return the enumerator object to the list to be used again, so on a second enumeration of the same list it wouldn't allocate.

May be issue if people hold on to the IEnumerable post foreach? However Reset is allowed to throw new NotSupportedException and is an explicitly implemented methodd, so the purpose of holding on to the reference would be limited.

[mikedn]

May be issue if people hold on to the IEnumerable post foreach?

How do you even know that the enumerator is used in a foreach statement? It's not a common scenario but people sometimes do use enumerators directly and when they do there's no way to know if they do it correctly.

var enumerator = Interlocked.Exchange(ref _cachedIEnumerator, null);

If I understand correctly this involves adding a new field to collection classes that use this technique. This penalizes those who use collection classes directly and not through interfaces because it increases the size of the collection object itself.

[benaadams]

@mikedn I dropped the extra field in PR dotnet/coreclr#4468

[mikedn]

I see, that's better.

In theory this looks like a good idea but it will probably be difficult to evaluated its true impact on performance. Keep in mind that you'll get different pools for different Ts and then the total memory used by these pools starts to add up. And of course, the cost of getting an object from a pool and then adding it back is not 0.

Anyway, it's a good subject for discussion.

[AndyAyersMS]

@benaadams can you update the status of this?

[benaadams]

Probably better as a single ThreadStatic cache per collection...

[benaadams]

Going to close this out

[benaadams]

A small step Zero alloc for empty IEnumerator<T> dotnet/coreclr#17187