From 1ceda71444850106c043d07457bcb0d9219e405c Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 30 Jun 2021 07:56:26 -0400 Subject: [PATCH] Remove some allocation from NotifyCollectionChangedEventArgs (#54899) * Remove some allocation from NotifyCollectionChangedEventArgs When initialized with a single object rather than a list of objects (single object is very common), the args is allocating an object[] and then wrapping that object[] in a ReadOnlyList. We can instead just allocate a simple read-only IList wrapper for the object directly; that also makes accessing it faster. Additionally, when constructing an instance using `public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList? changedItems, int index, int oldIndex)`, even though the same collection instance is used for both the new and old items, it wraps it twice. We can wrap it once. Along the way I also simplified the code, enabling the fields to become readonly, removing duplicate assignments of fields, etc. * Address PR feedback --- .../src/System.ObjectModel.csproj | 1 + .../System/Collections/CollectionHelpers.cs | 73 ++++ .../ObjectModel/ReadOnlyDictionary.cs | 88 +---- .../NotifyCollectionChangedEventArgs.cs | 332 ++++++++---------- 4 files changed, 220 insertions(+), 274 deletions(-) create mode 100644 src/libraries/System.ObjectModel/src/System/Collections/CollectionHelpers.cs diff --git a/src/libraries/System.ObjectModel/src/System.ObjectModel.csproj b/src/libraries/System.ObjectModel/src/System.ObjectModel.csproj index 89776eac649a5..66c53d0dfb08b 100644 --- a/src/libraries/System.ObjectModel/src/System.ObjectModel.csproj +++ b/src/libraries/System.ObjectModel/src/System.ObjectModel.csproj @@ -5,6 +5,7 @@ enable + diff --git a/src/libraries/System.ObjectModel/src/System/Collections/CollectionHelpers.cs b/src/libraries/System.ObjectModel/src/System/Collections/CollectionHelpers.cs new file mode 100644 index 0000000000000..8605820887328 --- /dev/null +++ b/src/libraries/System.ObjectModel/src/System/Collections/CollectionHelpers.cs @@ -0,0 +1,73 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; + +namespace System.Collections +{ + internal static class CollectionHelpers + { + internal static void ValidateCopyToArguments(int sourceCount, Array array, int index) + { + if (array == null) + { + throw new ArgumentNullException(nameof(array)); + } + + if (array.Rank != 1) + { + throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array)); + } + + if (array.GetLowerBound(0) != 0) + { + throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array)); + } + + if (index < 0 || index > array.Length) + { + throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_NeedNonNegNum); + } + + if (array.Length - index < sourceCount) + { + throw new ArgumentException(SR.Arg_ArrayPlusOffTooSmall); + } + } + + internal static void CopyTo(ICollection collection, Array array, int index) + { + ValidateCopyToArguments(collection.Count, array, index); + + if (collection is ICollection nonGenericCollection) + { + // Easy out if the ICollection implements the non-generic ICollection + nonGenericCollection.CopyTo(array, index); + } + else if (array is T[] items) + { + collection.CopyTo(items, index); + } + else + { + // We can't cast array of value type to object[], so we don't support widening of primitive types here. + if (array is not object?[] objects) + { + throw new ArgumentException(SR.Argument_InvalidArrayType, nameof(array)); + } + + try + { + foreach (T item in collection) + { + objects[index++] = item; + } + } + catch (ArrayTypeMismatchException) + { + throw new ArgumentException(SR.Argument_InvalidArrayType, nameof(array)); + } + } + } + } +} diff --git a/src/libraries/System.ObjectModel/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs b/src/libraries/System.ObjectModel/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs index 28b4c06533fa1..428b0663d263c 100644 --- a/src/libraries/System.ObjectModel/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs +++ b/src/libraries/System.ObjectModel/src/System/Collections/ObjectModel/ReadOnlyDictionary.cs @@ -175,26 +175,7 @@ void IDictionary.Remove(object key) void ICollection.CopyTo(Array array, int index) { - if (array == null) - { - throw new ArgumentNullException(nameof(array)); - } - if (array.Rank != 1) - { - throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array)); - } - if (array.GetLowerBound(0) != 0) - { - throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array)); - } - if (index < 0 || index > array.Length) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_NeedNonNegNum); - } - if (array.Length - index < Count) - { - throw new ArgumentException(SR.Arg_ArrayPlusOffTooSmall); - } + CollectionHelpers.ValidateCopyToArguments(Count, array, index); if (array is KeyValuePair[] pairs) { @@ -313,7 +294,7 @@ bool ICollection.Remove(TKey item) void ICollection.CopyTo(Array array, int index) { - ReadOnlyDictionaryHelpers.CopyToNonGenericICollectionHelper(_collection, array, index); + CollectionHelpers.CopyTo(_collection, array, index); } bool ICollection.IsSynchronized => false; @@ -363,7 +344,7 @@ bool ICollection.Remove(TValue item) void ICollection.CopyTo(Array array, int index) { - ReadOnlyDictionaryHelpers.CopyToNonGenericICollectionHelper(_collection, array, index); + CollectionHelpers.CopyTo(_collection, array, index); } bool ICollection.IsSynchronized => false; @@ -371,67 +352,4 @@ void ICollection.CopyTo(Array array, int index) object ICollection.SyncRoot => (_collection is ICollection coll) ? coll.SyncRoot : this; } } - - // To share code when possible, use a non-generic class to get rid of irrelevant type parameters. - internal static class ReadOnlyDictionaryHelpers - { - // Abstracted away to avoid redundant implementations. - internal static void CopyToNonGenericICollectionHelper(ICollection collection, Array array, int index) - { - if (array == null) - { - throw new ArgumentNullException(nameof(array)); - } - if (array.Rank != 1) - { - throw new ArgumentException(SR.Arg_RankMultiDimNotSupported, nameof(array)); - } - if (array.GetLowerBound(0) != 0) - { - throw new ArgumentException(SR.Arg_NonZeroLowerBound, nameof(array)); - } - if (index < 0) - { - throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_NeedNonNegNum); - } - if (array.Length - index < collection.Count) - { - throw new ArgumentException(SR.Arg_ArrayPlusOffTooSmall); - } - - // Easy out if the ICollection implements the non-generic ICollection - if (collection is ICollection nonGenericCollection) - { - nonGenericCollection.CopyTo(array, index); - return; - } - - if (array is T[] items) - { - collection.CopyTo(items, index); - } - else - { - // We can't cast array of value type to object[], so we don't support - // widening of primitive types here. - object?[]? objects = array as object?[]; - if (objects == null) - { - throw new ArgumentException(SR.Argument_InvalidArrayType, nameof(array)); - } - - try - { - foreach (var item in collection) - { - objects[index++] = item; - } - } - catch (ArrayTypeMismatchException) - { - throw new ArgumentException(SR.Argument_InvalidArrayType, nameof(array)); - } - } - } - } } diff --git a/src/libraries/System.ObjectModel/src/System/Collections/Specialized/NotifyCollectionChangedEventArgs.cs b/src/libraries/System.ObjectModel/src/System/Collections/Specialized/NotifyCollectionChangedEventArgs.cs index 3638daf718ef8..60b9898affd99 100644 --- a/src/libraries/System.ObjectModel/src/System/Collections/Specialized/NotifyCollectionChangedEventArgs.cs +++ b/src/libraries/System.ObjectModel/src/System/Collections/Specialized/NotifyCollectionChangedEventArgs.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Runtime.CompilerServices; namespace System.Collections.Specialized { @@ -14,11 +13,11 @@ namespace System.Collections.Specialized /// public class NotifyCollectionChangedEventArgs : EventArgs { - private NotifyCollectionChangedAction _action; - private IList? _newItems; - private IList? _oldItems; - private int _newStartingIndex = -1; - private int _oldStartingIndex = -1; + private readonly NotifyCollectionChangedAction _action; + private readonly IList? _newItems; + private readonly IList? _oldItems; + private readonly int _newStartingIndex = -1; + private readonly int _oldStartingIndex = -1; /// /// Construct a NotifyCollectionChangedEventArgs that describes a reset change. @@ -31,7 +30,7 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action) throw new ArgumentException(SR.Format(SR.WrongActionForCtor, NotifyCollectionChangedAction.Reset), nameof(action)); } - InitializeAdd(action, null, -1); + _action = action; } /// @@ -39,27 +38,9 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action) /// /// The action that caused the event; can only be Reset, Add or Remove action. /// The item affected by the change. - public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, object? changedItem) + public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, object? changedItem) : + this(action, changedItem, -1) { - if ((action != NotifyCollectionChangedAction.Add) && (action != NotifyCollectionChangedAction.Remove) - && (action != NotifyCollectionChangedAction.Reset)) - { - throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); - } - - if (action == NotifyCollectionChangedAction.Reset) - { - if (changedItem != null) - { - throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); - } - - InitializeAdd(action, null, -1); - } - else - { - InitializeAddOrRemove(action, new object?[] { changedItem }, -1); - } } /// @@ -70,29 +51,34 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, ob /// The index where the change occurred. public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, object? changedItem, int index) { - if ((action != NotifyCollectionChangedAction.Add) && (action != NotifyCollectionChangedAction.Remove) - && (action != NotifyCollectionChangedAction.Reset)) + switch (action) { - throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); + case NotifyCollectionChangedAction.Reset: + if (changedItem != null) + { + throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); + } + if (index != -1) + { + throw new ArgumentException(SR.ResetActionRequiresIndexMinus1, nameof(action)); + } + break; + + case NotifyCollectionChangedAction.Add: + _newItems = new SingleItemReadOnlyList(changedItem); + _newStartingIndex = index; + break; + + case NotifyCollectionChangedAction.Remove: + _oldItems = new SingleItemReadOnlyList(changedItem); + _oldStartingIndex = index; + break; + + default: + throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); } - if (action == NotifyCollectionChangedAction.Reset) - { - if (changedItem != null) - { - throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); - } - if (index != -1) - { - throw new ArgumentException(SR.ResetActionRequiresIndexMinus1, nameof(action)); - } - - InitializeAdd(action, null, -1); - } - else - { - InitializeAddOrRemove(action, new object?[] { changedItem }, index); - } + _action = action; } /// @@ -100,32 +86,9 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, ob /// /// The action that caused the event. /// The items affected by the change. - public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList? changedItems) + public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList? changedItems) : + this(action, changedItems, -1) { - if ((action != NotifyCollectionChangedAction.Add) && (action != NotifyCollectionChangedAction.Remove) - && (action != NotifyCollectionChangedAction.Reset)) - { - throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); - } - - if (action == NotifyCollectionChangedAction.Reset) - { - if (changedItems != null) - { - throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); - } - - InitializeAdd(action, null, -1); - } - else - { - if (changedItems == null) - { - throw new ArgumentNullException(nameof(changedItems)); - } - - InitializeAddOrRemove(action, changedItems, -1); - } } /// @@ -136,38 +99,47 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IL /// The index where the change occurred. public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList? changedItems, int startingIndex) { - if ((action != NotifyCollectionChangedAction.Add) && (action != NotifyCollectionChangedAction.Remove) - && (action != NotifyCollectionChangedAction.Reset)) - { - throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); - } - - if (action == NotifyCollectionChangedAction.Reset) + switch (action) { - if (changedItems != null) - { - throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); - } - if (startingIndex != -1) - { - throw new ArgumentException(SR.ResetActionRequiresIndexMinus1, nameof(action)); - } - - InitializeAdd(action, null, -1); + case NotifyCollectionChangedAction.Reset: + if (changedItems != null) + { + throw new ArgumentException(SR.ResetActionRequiresNullItem, nameof(action)); + } + if (startingIndex != -1) + { + throw new ArgumentException(SR.ResetActionRequiresIndexMinus1, nameof(action)); + } + break; + + case NotifyCollectionChangedAction.Add: + case NotifyCollectionChangedAction.Remove: + if (changedItems == null) + { + throw new ArgumentNullException(nameof(changedItems)); + } + if (startingIndex < -1) + { + throw new ArgumentException(SR.IndexCannotBeNegative, nameof(startingIndex)); + } + + if (action == NotifyCollectionChangedAction.Add) + { + _newItems = new ReadOnlyList(changedItems); + _newStartingIndex = startingIndex; + } + else + { + _oldItems = new ReadOnlyList(changedItems); + _oldStartingIndex = startingIndex; + } + break; + + default: + throw new ArgumentException(SR.MustBeResetAddOrRemoveActionForCtor, nameof(action)); } - else - { - if (changedItems == null) - { - throw new ArgumentNullException(nameof(changedItems)); - } - if (startingIndex < -1) - { - throw new ArgumentException(SR.IndexCannotBeNegative, nameof(startingIndex)); - } - InitializeAddOrRemove(action, changedItems, startingIndex); - } + _action = action; } /// @@ -176,14 +148,9 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IL /// Can only be a Replace action. /// The new item replacing the original item. /// The original item that is replaced. - public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, object? newItem, object? oldItem) + public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, object? newItem, object? oldItem) : + this(action, newItem, oldItem, -1) { - if (action != NotifyCollectionChangedAction.Replace) - { - throw new ArgumentException(SR.Format(SR.WrongActionForCtor, NotifyCollectionChangedAction.Replace), nameof(action)); - } - - InitializeMoveOrReplace(action, new object?[] { newItem }, new object?[] { oldItem }, -1, -1); } /// @@ -200,7 +167,10 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, ob throw new ArgumentException(SR.Format(SR.WrongActionForCtor, NotifyCollectionChangedAction.Replace), nameof(action)); } - InitializeMoveOrReplace(action, new object?[] { newItem }, new object?[] { oldItem }, index, index); + _action = action; + _newItems = new SingleItemReadOnlyList(newItem); + _oldItems = new SingleItemReadOnlyList(oldItem); + _newStartingIndex = _oldStartingIndex = index; } /// @@ -209,22 +179,9 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, ob /// Can only be a Replace action. /// The new items replacing the original items. /// The original items that are replaced. - public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList newItems, IList oldItems) + public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList newItems, IList oldItems) : + this(action, newItems, oldItems, -1) { - if (action != NotifyCollectionChangedAction.Replace) - { - throw new ArgumentException(SR.Format(SR.WrongActionForCtor, NotifyCollectionChangedAction.Replace), nameof(action)); - } - if (newItems == null) - { - throw new ArgumentNullException(nameof(newItems)); - } - if (oldItems == null) - { - throw new ArgumentNullException(nameof(oldItems)); - } - - InitializeMoveOrReplace(action, newItems, oldItems, -1, -1); } /// @@ -249,7 +206,10 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IL throw new ArgumentNullException(nameof(oldItems)); } - InitializeMoveOrReplace(action, newItems, oldItems, startingIndex, startingIndex); + _action = action; + _newItems = new ReadOnlyList(newItems); + _oldItems = new ReadOnlyList(oldItems); + _newStartingIndex = _oldStartingIndex = startingIndex; } /// @@ -270,8 +230,10 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, ob throw new ArgumentException(SR.IndexCannotBeNegative, nameof(index)); } - object?[] changedItems = new object?[] { changedItem }; - InitializeMoveOrReplace(action, changedItems, changedItems, index, oldIndex); + _action = action; + _newItems = _oldItems = new SingleItemReadOnlyList(changedItem); + _newStartingIndex = index; + _oldStartingIndex = oldIndex; } /// @@ -292,54 +254,12 @@ public NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IL throw new ArgumentException(SR.IndexCannotBeNegative, nameof(index)); } - InitializeMoveOrReplace(action, changedItems, changedItems, index, oldIndex); - } - - /// - /// Construct a NotifyCollectionChangedEventArgs with given fields (no validation). Used by WinRT marshaling. - /// - internal NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction action, IList? newItems, IList? oldItems, int newIndex, int oldIndex) - { _action = action; - _newItems = (newItems == null) ? null : new ReadOnlyList(newItems); - _oldItems = (oldItems == null) ? null : new ReadOnlyList(oldItems); - _newStartingIndex = newIndex; + _newItems = _oldItems = changedItems is not null ? new ReadOnlyList(changedItems) : null; + _newStartingIndex = index; _oldStartingIndex = oldIndex; } - private void InitializeAddOrRemove(NotifyCollectionChangedAction action, IList? changedItems, int startingIndex) - { - if (action == NotifyCollectionChangedAction.Add) - { - InitializeAdd(action, changedItems, startingIndex); - } - else - { - Debug.Assert(action == NotifyCollectionChangedAction.Remove, $"Unsupported action: {action}"); - InitializeRemove(action, changedItems, startingIndex); - } - } - - private void InitializeAdd(NotifyCollectionChangedAction action, IList? newItems, int newStartingIndex) - { - _action = action; - _newItems = (newItems == null) ? null : new ReadOnlyList(newItems); - _newStartingIndex = newStartingIndex; - } - - private void InitializeRemove(NotifyCollectionChangedAction action, IList? oldItems, int oldStartingIndex) - { - _action = action; - _oldItems = (oldItems == null) ? null : new ReadOnlyList(oldItems); - _oldStartingIndex = oldStartingIndex; - } - - private void InitializeMoveOrReplace(NotifyCollectionChangedAction action, IList? newItems, IList? oldItems, int startingIndex, int oldStartingIndex) - { - InitializeAdd(action, newItems, startingIndex); - InitializeRemove(action, oldItems, oldStartingIndex); - } - /// /// The action that caused the event. /// @@ -397,40 +317,74 @@ public object? this[int index] public object SyncRoot => _list.SyncRoot; - public int Add(object? value) - { - throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); - } + public int Add(object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); - public void Clear() - { - throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); - } + public void Clear() => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); public bool Contains(object? value) => _list.Contains(value); - public void CopyTo(Array array, int index) - { - _list.CopyTo(array, index); - } + public void CopyTo(Array array, int index) => _list.CopyTo(array, index); public IEnumerator GetEnumerator() => _list.GetEnumerator(); public int IndexOf(object? value) => _list.IndexOf(value); - public void Insert(int index, object? value) + public void Insert(int index, object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + + public void Remove(object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + + public void RemoveAt(int index) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + } + + internal sealed class SingleItemReadOnlyList : IList + { + private readonly object? _item; + + public SingleItemReadOnlyList(object? item) => _item = item; + + public object? this[int index] { - throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + get + { + if (index != 0) + { + throw new ArgumentOutOfRangeException(nameof(index)); + } + + return _item; + } + set => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); } - public void Remove(object? value) + public bool IsFixedSize => true; + + public bool IsReadOnly => true; + + public int Count => 1; + + public bool IsSynchronized => false; + + public object SyncRoot => this; + + public IEnumerator GetEnumerator() { - throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + yield return _item; } - public void RemoveAt(int index) + public bool Contains(object? value) => _item is null ? value is null : _item.Equals(value); + + public int IndexOf(object? value) => Contains(value) ? 0 : -1; + + public void CopyTo(Array array, int index) { - throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + CollectionHelpers.ValidateCopyToArguments(1, array, index); + array.SetValue(_item, index); } + + public int Add(object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + public void Clear() => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + public void Insert(int index, object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + public void Remove(object? value) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); + public void RemoveAt(int index) => throw new NotSupportedException(SR.NotSupported_ReadOnlyCollection); } }