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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃毎 Menu component listener leak #761

Closed
samccone opened this Issue Jul 6, 2015 · 4 comments

Comments

Projects
None yet
5 participants
@samccone
Member

samccone commented Jul 6, 2015

Hypothesis: The menu when opened and closed does not clean up all of the events that it registers for, thus causing a leak 馃挧 .

screen shot 2015-07-06 at 4 04 06 pm

#### Testing methodology:

*visit http://www.getmdl.io/components/index.html#menus-section in chrome canary in an incognito window with no profile

  • open and close the left menu once
  • take a heap snapshot
  • start a profiler (clearing the GC after starting)
  • open and close the menu 5 times
  • force a GC
  • stop the timeline profile
  • take a heap snapshot.

Results:

screen shot 2015-07-06 at 4 07 45 pm

The profiler shows us a step ladder of listener registrations (that never drops), over 5 iterations we can see that there are 15 new event registrations.

Looking at the heap comparison

screen shot 2015-07-06 at 4 03 55 pm

note how that 15 shows up for us :) it looks like here 鈽濓笍 is the manifestation of our leak 馃憦


I have not investigated the menu code in depth for how the menu works, so this might be super obvious to you guys for how to fix (or even that this might not be an issue), if this is not enough info to go off of let me know and I can continue the dive.

馃尨

@nicolasgarnier nicolasgarnier added the P1 label Jul 6, 2015

@samccone samccone changed the title from 馃毎Menu component listener leak to 馃毎 Menu component listener leak Jul 6, 2015

@Garbee

This comment has been minimized.

Show comment
Hide comment
@Garbee

Garbee Jul 6, 2015

Collaborator

I think I found the leak. We are adding a listener to animate when hidden, but they are never removed. Therefore when it is opened repeatedly, the same events keep getting added again and again.

Collaborator

Garbee commented Jul 6, 2015

I think I found the leak. We are adding a listener to animate when hidden, but they are never removed. Therefore when it is opened repeatedly, the same events keep getting added again and again.

@surma

This comment has been minimized.

Show comment
Hide comment
@surma

surma Jul 6, 2015

Member

That鈥檚 one pretty issue you got there! Thank you very much for the in-depth analysis. Is either of you working on a PR already?

Member

surma commented Jul 6, 2015

That鈥檚 one pretty issue you got there! Thank you very much for the in-depth analysis. Is either of you working on a PR already?

@Garbee

This comment has been minimized.

Show comment
Hide comment
@Garbee

Garbee Jul 6, 2015

Collaborator

I'm not working on a PR for this, much lower priority than my card goof that is very problematic for alignment. Go for fixing it if you want.

(Yea, he has been doing loads of good reviews like this for TodoMVC stuff.)

Collaborator

Garbee commented Jul 6, 2015

I'm not working on a PR for this, much lower priority than my card goof that is very problematic for alignment. Go for fixing it if you want.

(Yea, he has been doing loads of good reviews like this for TodoMVC stuff.)

@surma surma self-assigned this Jul 6, 2015

surma added a commit that referenced this issue Jul 6, 2015

@surma surma closed this in 957656d Jul 7, 2015

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Jul 7, 2015

Contributor

猸愶笍 Thanks again, @samccone!

Contributor

addyosmani commented Jul 7, 2015

猸愶笍 Thanks again, @samccone!

samccone added a commit that referenced this issue Dec 9, 2015

Utilize event deduping with addEventListener
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.

:)

TheOnlyMerlin pushed a commit to TheOnlyMerlin/material-design-lite-src that referenced this issue May 20, 2017

Utilize event deduping with addEventListener
The initial leak found here
google/material-design-lite#761

And then patched here
google/material-design-lite@957656d

Was never the "correct" fix. Yes the patch in
957656d2beaab2d990b7758c8f13f44f9d0dfaf3 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.

:)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment