Utilize event deduping optmization with addEventListener #1965
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The initial leak found here #761 And then patched here 957656d Was never the "correct" fix. Yes the patch in 957656d cause us to no longer leak however we were still paying the cost of registering and unregistering for the same event over and over, as well as allocating a new bound scope each time (thus generating more garbage and thus eventually causing more time to be spent int GC) Instead I opted here to take advantage of addEventListener and its ability to automatically discard duplicate listeners. > If multiple identical EventListeners are registered on the same > EventTarget with the same parameters, the duplicate instances are > discarded. They do not cause the EventListener to be called twice, and > they do not need to be removed manually with the removeEventListener > method. https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Multiple_identical_event_listeners Because there is no outside pointers into this event, there is no need to remove the listener anymore, because once the node reference becomes unreachable the events will be automatically GCd for us. :)
|
Great work, Sam. This initially LGTM |
|
LGTM :) Awesome stuff 🎉 @sgomes Imma leave it to you to decide whether to merge |
|
Looks 1.1-worthy to me. Thanks @samccone! :) |
sgomes
added a commit
that referenced
this pull request
Dec 9, 2015
Utilize event deduping optmization with addEventListener
|
🙇 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR; ✨ ❤️
The old fix for the leak, did accomplish removing the event listener leak, however it was fixed in a way that caused object thrashing due to the non-uniqueness of each event-listener bound fn (due to the use of native bind)
The initial leak found here #761
And then patched here 957656d
Was never the "correct" fix. Yes the patch in
957656d caused us to no longer leak
however we were still paying the cost of registering and unregistering
for the same event over and over, as well as allocating a new bound
scope each time (thus generating more garbage and thus eventually
causing more time to be spent in GC)
Instead I opted here to take advantage of addEventListener and its
ability to automatically discard duplicate listeners. (1)
Resulting in a much more efficient use of runtime memory heap allocations (and thus less time to be spent in GC land)
💛
Because there is no outside pointers into this eventListener (such as
.bindor a parent self reference), there is no need to remove the listener anymore, because once the node reference becomesunreachable the events will be automatically GCd for us. 🎲
(1)