I've an IEnumerable<T> source which takes time to generate each item. Once an item is generated, I want to cache it to avoid recomputing the same thing. I want to separate out caching logic from original source.

A few things that bother me are:

  1. Where should I have _cache? In CachedEnumerable or CacheEnumerator?
  2. Thread safety in adding into cache.
  3. Reinventing? Any existing .NET or other library which does this.
  4. Why Dispose in IEnumerator? Should I be doing something there? _cache nuke?

class CachedEnumerable<T> : IEnumerable<T>
{
    private IList<T> _cache = new List<T>();
    IEnumerator<T> _cacheEnumerator = null;
    IEnumerable<T> _source = null;

    public CachedEnumerable(IEnumerable<T> source)
    {
        _source = source;
        _cacheEnumerator = _source.GetEnumerator();
    }

    public IEnumerator<T> GetEnumerator()
    {
        return new CachedEnumerator<T>(_cache, _cacheEnumerator);
    }

    System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

class CachedEnumerator<T> : IEnumerator<T>
{
    private IList<T> _cache;
    private IEnumerator<T> _source;
    private T _current;
    private int _index;
    private bool _moveNextStatus;

    public CachedEnumerator(IList<T> cache, IEnumerator<T> source)
    {
        _cache = cache;
        _source = source;
        Reset();
    }

    public T Current
    {
        get
        {
            if (_moveNextStatus)
            {
                return _current;
            }
            else
            {
                throw new InvalidOperationException();
            }
        }
    }

    public void Dispose()
    {
        _source.Dispose();
    }

    object System.Collections.IEnumerator.Current
    {
        get { return Current; }
    }

    public bool MoveNext()
    {
        _moveNextStatus = _MoveNext();
        return _moveNextStatus;
    }

    private bool _MoveNext()
    {
        _index++;

        if (_index < _cache.Count)
        {
            _current = _cache[_index];
            return true;
        }
        else
        {
            if (_source.MoveNext())
            {
                _current = _source.Current;
                _cache.Add(_current);
                return true;
            }
            else
            {
                return false;
            }
        }
    }

    public void Reset()
    {
        _index = -1;
        _current = default(T);
        _moveNextStatus = false;
    }
}
  • 3
    The idea of IEnumerable is that it is lazy and executed on command. If you want to cache data, then why not put it in a List? You can then write further queries onto your list. – Aphelion May 8 '12 at 9:46
  • 1
    A bit late to the party, but here's an implementation that I found that was done quite well. It is thread safe, lazy, and licensed under MS-PL. gist.github.com/AArnott/118348 It's got some nice advantages over your code, such as not caching the IEnumerable if it's already a list, array, etc. Only calling GetEnumerator in the CachedEnumerator's GetEnumerator instead of the constructor, implements Dispose with more intuitive behavior, and, as mentioned before, is thread safe – Marty Neal Mar 5 '13 at 14:52
  1. You need to access _cache in both classes, so it makes sense to have it as field in both of them too.
  2. Your code is not thread safe. If you wanted to make it thread safe, I think the best solution would be to have a lock that you use whenever you access _cache or _source.
  3. I don't think anything like this is in .Net.
  4. What you're currently doing in Dispose() is wrong. You can't call Dispose() on _source and expect to use it later in another enumerator. In your case, I think you shouldn't do anything in Dispose(). You certainly shouldn't “nuke” _cache, because you're going to need it later!
  • about 4. So my CachedEnumerable<T> needs to inherit from IDisposable and clear cache and dispose cacheEnumerator. Right? – R56 May 6 '12 at 12:33
  • @R56, yeah implementing IDisposable in CachedEnumerable makes sense. Clearing the cache there not so much, because that's just managed memory and the GC will take of that. – svick May 6 '12 at 13:01

1 and 2 I agree with svick:

  1. You need to access _cache in both classes, so it makes sense to have it as field in both of them too.
  2. Your code is not thread safe. If you wanted to make it thread safe, I think the best solution would be to have a lock that you use whenever you access _cache or _source.

Regarding 3 I found the following 2 solutions besides this page:

You do not have to go to all this trouble to implement a caching wrapper. It can be done much more simply:

class CachedEnumerable<T> : IEnumerable<T> 
{
    private readonly IEnumerable<T> producer;
    private IEnumerable<T> cache;

    public CachedEnumerable(IEnumerable<T> producer)
    {
        this.producer = producer;
    }

    public IEnumerator<T> GetEnumerator()
    {
        if (!this.IsCacheValid()) {
            this.cache = this.producer.ToList();
        }

        return this.cache.GetEnumerator();
    }

    protected virtual bool IsCacheValid()
    {
        return this.cache != null;
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return this.GetEnumerator();
    }
}

This implementation has a cache that never expires, but you can modify (or override in a derived class) IsCacheValid to tweak this behavior to taste. It also has no thread safety, but that's as easy to add as

// need to make this always have a non-null value so it can be 
// used as a parameter to Monitor.Enter
private IEnumerable<T> cache = new T[0];

public IEnumerator<T> GetEnumerator()
{
    lock (this.cache) {
        if (!this.IsCacheValid()) {
            this.cache = this.producer.ToList();
        }

        return this.cache.GetEnumerator();
    }
}

If IsCacheValid were private then the lock could be reduced to protecting just the this.producer.ToList() statement, but as it stands it must protect everything inside GetEnumerator.

  • 2
    I can't invoke this.producer.ToList() This will take a lot of time. That's why I'm going through the trouble to MoveNext(). Caller's of this API usually call this using producer.Take(n) – R56 May 6 '12 at 5:36

Your Answer

 
discard

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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