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

perf: null out els on dispose to minimize detached els #4745

Merged
merged 18 commits into from Nov 16, 2017

Conversation

Projects
None yet
2 participants
@gkatsev
Member

gkatsev commented Nov 13, 2017

Description

We have a bunch of references to DOM elements we were retaining even after we had disposed the player. This cleans up a bunch of them.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors
@misteroneill

The changes LGTM, but I wonder if we ought to implement something like a conventional nulling-out of all *El_ and *Node_ properties in Component#dispose?

Object.keys(this).forEach(k => {
  if ((/(El|Node)_$/).test(k)) {
    this[k] = null;
  }
});

Maybe that's too magical, but thought I'd throw it out there.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 14, 2017

Member

Yeah, I think that's a bit too magic and will just mean that if we do something else we will think we're safe when we're not.

Member

gkatsev commented Nov 14, 2017

Yeah, I think that's a bit too magic and will just mean that if we do something else we will think we're safe when we're not.

@misteroneill

Yep, that's fine. 👍

@gkatsev

Basically no elements are retained anymore.
Though, there's still a weird issue where elData in dom-data.js has two entries that are empty after disposing the player.

// Make this an evented object and use `el_`, if available, as its event bus
evented(this, {eventBusKey: this.el_ ? 'el_' : null});
// if evented is anything except false, we want to mixin in evented
if (options.evented !== false) {

This comment has been minimized.

@gkatsev

gkatsev Nov 14, 2017

Member

this PR is still a perf but adds this feature to disable auto evented mixin.

@gkatsev

gkatsev Nov 14, 2017

Member

this PR is still a perf but adds this feature to disable auto evented mixin.

This comment has been minimized.

@misteroneill

misteroneill Nov 16, 2017

Member

Good idea. 👍

@misteroneill

misteroneill Nov 16, 2017

Member

Good idea. 👍

target.on('dispose', () => {
target.off();
window.setTimeout(() => {
target.eventBusEl_ = null;

This comment has been minimized.

@gkatsev

gkatsev Nov 14, 2017

Member

we want to null out this reference so that the reference isn't retained when a component is disposed.

@gkatsev

gkatsev Nov 14, 2017

Member

we want to null out this reference so that the reference isn't retained when a component is disposed.

data.handlers[t] = [];
_cleanUpEvents(elem, t);

This comment has been minimized.

@gkatsev

gkatsev Nov 14, 2017

Member

makes it more likely that elem isn't accidentally kept in the closure.

@gkatsev

gkatsev Nov 14, 2017

Member

makes it more likely that elem isn't accidentally kept in the closure.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 14, 2017

Member

Looks like the latest set of changes broke some tests.

Member

gkatsev commented Nov 14, 2017

Looks like the latest set of changes broke some tests.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 14, 2017

Member

IE8 tests can actually finish! even if they might run out of memory on a run, it'll be fine the next run. Will look at the broken tests soon.

Member

gkatsev commented Nov 14, 2017

IE8 tests can actually finish! even if they might run out of memory on a run, it'll be fine the next run. Will look at the broken tests soon.

gkatsev added some commits Nov 15, 2017

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Nov 16, 2017

Member

YES. All tests passed and with just one IE8 run!

Member

gkatsev commented Nov 16, 2017

YES. All tests passed and with just one IE8 run!

// Make this an evented object and use `el_`, if available, as its event bus
evented(this, {eventBusKey: this.el_ ? 'el_' : null});
// if evented is anything except false, we want to mixin in evented
if (options.evented !== false) {

This comment has been minimized.

@misteroneill

misteroneill Nov 16, 2017

Member

Good idea. 👍

@misteroneill

misteroneill Nov 16, 2017

Member

Good idea. 👍

@gkatsev gkatsev merged commit 2da7af1 into master Nov 16, 2017

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@gkatsev gkatsev deleted the detached-els branch Nov 16, 2017

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