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

Avoid leaking DOM objects #9067

Merged
merged 4 commits into from Oct 7, 2015

Conversation

Projects
None yet
3 participants
@as-cii
Member

as-cii commented Oct 7, 2015

With this PR we make sure to clear out all the references that caused TextEditorComponents to be retained (e.g. closures, etc.). Below you can have a look at how the heap looked like after scrolling for a while on multiple editors and then closing them.

screen shot 2015-10-07 at 11 09 04

screen shot 2015-10-07 at 11 16 35

As you can see, a lot of elements were leaking because they were still retained by TextEditorComponent.

This is how the heap looks after the changes in this PR:

screen shot 2015-10-07 at 11 09 11

馃毐 馃毐 馃毐

/cc: @nathansobo @atom/feedback for 馃憖

@as-cii as-cii added the needs-review label Oct 7, 2015

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 7, 2015

Contributor

This seems fine to me, though it would like to find out why we're leaking the model to begin with. But I think this is actually a pretty good mitigating strategy in general... when something bulky like a text editor gets destroyed, null out all of its components. That way if anyone leaks a reference, the cost will be much lower... just a small wrapper object.

Contributor

nathansobo commented Oct 7, 2015

This seems fine to me, though it would like to find out why we're leaking the model to begin with. But I think this is actually a pretty good mitigating strategy in general... when something bulky like a text editor gets destroyed, null out all of its components. That way if anyone leaks a reference, the cost will be much lower... just a small wrapper object.

nathansobo added a commit that referenced this pull request Oct 7, 2015

Merge pull request #9067 from atom/as-memory-leaks
Avoid leaking DOM objects

@nathansobo nathansobo merged commit 993419f into master Oct 7, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nathansobo nathansobo deleted the as-memory-leaks branch Oct 7, 2015

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Oct 8, 2015

Member

though it would like to find out why we're leaking the model to begin with

Ugh, I guess my comment was stripped off by an unfortunate diff mismatch. (#9067 (comment)) Sorry about that.

By the way, my suspects point towards a problem with the GC or Coffeescript causing the cyclic reference not to be resolved correctly. What do you think?

Member

as-cii commented Oct 8, 2015

though it would like to find out why we're leaking the model to begin with

Ugh, I guess my comment was stripped off by an unfortunate diff mismatch. (#9067 (comment)) Sorry about that.

By the way, my suspects point towards a problem with the GC or Coffeescript causing the cyclic reference not to be resolved correctly. What do you think?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Oct 8, 2015

Contributor

Very skeptical of a bug in the garbage collector. I've always found a reason but it's usually hard.

Contributor

nathansobo commented Oct 8, 2015

Very skeptical of a bug in the garbage collector. I've always found a reason but it's usually hard.

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