Implementing IList<T>, IList & INotifyCollectionChanged

Question

I posted this question over at Stackoverflow, which was off-topic.

I was using ObservableCollection in a number of places in my WPF application for very large arrays. This caused a problem as ObservableCollection did not take an argument for capacity like List did. This was an important optimization for locking down my memory usage and ensuring that these collections did not double to a much larger size than required.

So I implemented the following class:

public class ObservableList<T> : IList<T>, IList, INotifyCollectionChanged
{
 public event NotifyCollectionChangedEventHandler CollectionChanged;
 #region Properties
 private List<T> _List
 {
 get;
 set;
 }
 public T this[int index]
 {
 get
 {
 return _List[index];
 }
 set
 {
 if (index < 0 || index >= Count)
 throw new IndexOutOfRangeException("The specified index is out of range.");
 var oldItem = _List[index];
 _List[index] = value;
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, value, oldItem, index));
 }
 }
 public int Count
 {
 get
 {
 return _List.Count;
 }
 }
 public bool IsReadOnly
 {
 get
 {
 return ((IList<T>)_List).IsReadOnly;
 }
 }
 bool IList.IsFixedSize
 {
 get
 {
 return ((IList)_List).IsFixedSize;
 }
 }
 object IList.this[int index]
 {
 get
 {
 return ((IList)_List)[index];
 }
 set
 {
 if (index < 0 || index >= Count)
 throw new IndexOutOfRangeException("The specified index is out of range.");
 var oldItem = ((IList)_List)[index];
 ((IList)_List)[index] = value;
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Replace, value, oldItem, index));
 }
 }
 public bool IsSynchronized
 {
 get
 {
 return ((IList)_List).IsSynchronized;
 }
 }
 public object SyncRoot
 {
 get
 {
 return ((IList)_List).SyncRoot;
 }
 }
 #endregion
 #region Methods
 private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
 {
 if (CollectionChanged != null)
 CollectionChanged(this, args);
 }
 public void AddRange(IEnumerable<T> collection)
 {
 _List.AddRange(collection);
 var iList = collection as IList;
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
 }
 public int IndexOf(T item)
 {
 return _List.IndexOf(item);
 }
 public void Insert(int index, T item)
 {
 _List.Insert(index, item);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item, index));
 }
 public void RemoveAt(int index)
 {
 if (index < 0 || index >= Count)
 throw new IndexOutOfRangeException("The specified index is out of range.");
 var oldItem = _List[index];
 _List.RemoveAt(index);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, oldItem, index));
 }
 public void Add(T item)
 {
 _List.Add(item);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, item));
 }
 public void Clear()
 {
 _List.Clear();
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
 }
 public bool Contains(T item)
 {
 return _List.Contains(item);
 }
 public void CopyTo(T[] array, int arrayIndex)
 {
 _List.CopyTo(array, arrayIndex);
 }
 public bool Remove(T item)
 {
 var result = _List.Remove(item);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, item));
 return result;
 }
 public IEnumerator<T> GetEnumerator()
 {
 return _List.GetEnumerator();
 }
 IEnumerator IEnumerable.GetEnumerator()
 {
 return GetEnumerator();
 }
 int IList.Add(object value)
 {
 var result = ((IList)_List).Add(value);
 ;
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, value));
 return result;
 }
 bool IList.Contains(object value)
 {
 return ((IList)_List).Contains(value);
 }
 int IList.IndexOf(object value)
 {
 return ((IList)_List).IndexOf(value);
 }
 void IList.Insert(int index, object value)
 {
 ((IList)_List).Insert(index, value);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Add, value, index));
 }
 void IList.Remove(object value)
 {
 ((IList)_List).Remove(value);
 OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Remove, value));
 }
 public void CopyTo(Array array, int index)
 {
 ((IList)_List).CopyTo(array, index);
 }
 #endregion
 #region Initialization
 public ObservableList()
 {
 _List = new List<T>();
 }
 public ObservableList(int capacity)
 {
 _List = new List<T>(capacity);
 }
 public ObservableList(IEnumerable<T> collection)
 {
 _List = new List<T>(collection);
 }
 #endregion
}

It seems to be working fine, but I have noticed an increase in processing time since switching to this implementation. Have I implemented the interfaces correctly?

• dreza: Can you compare the two pieces of code. Perhaps it's just a red herring the increase in processing occurring around the same time as your code change?
• Robert Gowland: Further to @dreza's comment, to help make sure that inefficiency is related to this change, you'll want to do some benchmarking and profiling. Eric Lippert has an excellent series on this start here (tech.pro/blog/1293/c-performance-benchmark-mistakes-part-one). As far as anything obvious with the code itself that may be causing a slow down... the casting (((IList)_List)) throughout looks like a possible suspect to me.
• kornman00: FWIW ObservableCollection has a ctor which takes a List<T> (msdn.microsoft.com/en-us/library/ms653202(v=vs.110).aspx), albeit it copies the elements. You could perform the list generation up front, then create the OC from that list. You can also use reflection/LINQ Expressions to modify the Capacity of Collection<T>'s internal IList (which is a List by default referencesource.microsoft.com/#mscorlib/system/collections/…). Less code to maintain and test.
• kornman00: Also, both your implementation and .NET's ObservableCollection will always allocate the ThingChangedEvent objects for all events, even when there are no subscribers. If you were populating the OC directly, with elements one at a time, it may be hurting your performance as well (GC news/collects are not cheap).

Answer 1

1. The implementation of IList is inconsistent: You have implemented IsFixedSize explicitly while IsSynchronized and SyncRoot are not, yet they are all three part of the IList interface. You should probably implement IList entirely explicitly.
2. You occasionally do some unnecessary casting, like here ((IList)_List)[index]. Only cast if necessary (e.g. when casting the underlying List<T> into IList because you need access to some explicitly implemented interface methods/properties).

3. There is a potential race condition in your event raising method:

   private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
   {
    if (CollectionChanged != null)
    CollectionChanged(this, args);
   }
   

   If the subscriber unsubscribes after your if but before the actual call you will get a NullReferenceException. The idiomatic C# way of implementing this is:

   private void OnCollectionChanged(NotifyCollectionChangedEventArgs args)
   {
    var handler = CollectionChanged;
    if (handler != null)
    handler(this, args);
   }
   

   Note that this can still pose a problem on the subscriber side because it might get called just after it has unsubscribed but it should be dealt with there.

4. Calling event handlers happens synchronously. So if something has subscribed to the CollectionChanged event then the registered event handlers will be called synchronously on every operation raising the event. It is not totally surprising that this slows down the operation a bit. In the unobserved case however it should not make a huge difference.
5. To truly track down where the problem is you'll have to use a profiler.

• svick: Partial explicit implementation can make sense, especially for members that are more or less obsolete, like SyncRoot (I realize that in the original code, this one is not explicit). And it's actually necessary in the case of this[].
• ChrisWue: @svick: Hm, my point was that he should probably implement the entire IList explicitly. Bringing more or less obsolete members back to life is not really a good thing, is it?