-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to a HashSet<T> as backing for SafeEnumerableList #16633
Conversation
You can test this PR using the following package version. |
|
@cla-avalonia agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this change makes perfect sense.
Thank you for your contribution!
Note that this PR isn't safe to backport since it technically changes the enumeration order, which can have some effects on the way styles get attached/detached. |
What does the pull request do?
Switches
SafeEnumerableList
toSafeEnumerableSet
, in order to offer better performance on the.Add
and.Remove
methodsWhat is the current behavior?
As the
Classes
class adds and removes listeners it stores them in aSafeEnumerableList
. When listeners are removed they are removed from this list. However, since the backing store for this collection is aList<T>
the.Remove
method becomes a O(n) scan on the items. In our application we were seeing significant pauses in the application when the number of listeners grew rather large. In our case one of the lists had about 700 listeners, so the pauses became quite large.In addition, if it is assumed that listeners that are added recently (like those deep in a tree) will be removed first, we can see another problem: the items we remove will most likely be at the end of the collection.
What is the updated/expected behavior with this PR?
The backing store is now a HashSet. None of the public APIs have changed in behavior, although there may be slight differences in performance, similar to those expected from switching from a list to a hashset.
How was the solution implemented (if it's not obvious)?
Pretty much the same as the previous solution except for the following changes:
_list[idx]
inside theEnumerable
class as HashSets don't support offset indexing. Instead when we create the enumerable we wrap the specialized .NET HashSet enumerator, and add our functionality to this method. This also means that we no longer need a few of the fields onEnumerable
HashSet
if it is modified during enumeration. This is supported vianew HashSet(_set)
which has an optimized fastpath in the .NET Constructor. It is true that cloning a hashset is slower than cloning an array, but it is assumed that the "modified while enumerating" usecases are rare.Checklist