Skip to content
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

[BUGFIX release] Fix Glimmer memory leak #11667

Merged
merged 7 commits into from Jul 8, 2015

Conversation

Projects
None yet
9 participants
@tomdale
Copy link
Member

commented Jul 6, 2015

This commit resolves a memory leak (#11501) in apps running Glimmer. The root cause of the issue was that a flag governing whether components/views remove themselves from their parent was being set incorrectly, so that when the code to cleanup a destroyed view ran, it was not removed from its parent’s childViews array.

Specifically, three hooks are invoked when a render node is cleared:

  1. env.hooks.willCleanupNode
  2. Morph#cleanup
  3. env.hooks.didCleanupNode

Prior to this commit, willCleanupNode would blindly set the owner view’s isDestroyingSubtree flag if there was a view set in the env (i.e., basically always).

Sidebar on the isDestroyingSubtree flag: this flag is used as a performance optimization. If a view is destroyed, we want to remove that view from its parent’s childViews array so that garbage collection can happen. However, it is unnecessary to remove child views from the destroyed view’s childViews array; the GC will take care of any clean up when the link between the view hierarchy and the destroyed view is severed.

For example, imagine this hypothetical view hierarchy:

      A
     / \
    B   C
       / \
      D   E

If the render node for view C is destroyed, we need to remove C from A’s array of child views. However, it is unnecessary to remove D or E from C’s child views, because they will be imminently removed by the GC, and temporarily lingering in the child views array does no harm.

We accomplish this by setting the isDestroyingSubtree flag on the root-most view in a hierarchy (view A in the example above), which we call the owner view. When the render node for C is destroyed, it removes C from A’s child views array and sets the flag to true. When D and E’s render nodes are destroyed, they see that the flag has already been flipped and do not remove their associated views from C’s child views array.

The memory leak manifested itself when the a render node that did not have a view associated with it was destroyed, and contained a child render node that did have a view associated. For example:

{{#if foo}}
  {{my-component}}
{{/if}}

In this case, the willCleanupNode hook would erroneously set the flag without actually moving the my-component view, because the render node for the {{if}} does not know about the component.

When the cleanup for {{my-component}} finally happens (in Morph#cleanup), the flag has already been set, so the render node incorrectly assumes that its view is part of a subgraph that has already been severed.

As far as we can tell, the willCleanupNode hook is not doing any cleanup that is not already done in Morph#cleanup. We still do need didCleanupNode to clear the flag, but can leave individual render node cleanup to the render node itself.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 6, 2015

+1 bugfix that kills removes features, it seems like the memory leak originated behind the keyboard, accidentally retaining some extra code :trollface:

@tomdale tomdale changed the title [BUGFIX beta] Fix Glimmer memory leak [BUGFIX release] Fix Glimmer memory leak Jul 6, 2015

@igorT

View changes

packages/ember-views/tests/views/view/child_views_test.js Outdated
return {
componentFor(tagName, container) {
return Component.extend({
destroy() {

This comment has been minimized.

Copy link
@igorT

igorT Jul 6, 2015

Member

Why do we need this?

This comment has been minimized.

Copy link
@rwjblue

rwjblue Jul 6, 2015

Member

We should probably add an assertion here, to confirm that destroy is called...

This comment has been minimized.

Copy link
@tomdale

tomdale Jul 7, 2015

Author Member

Removed, thanks for catching that.

Eliminate redundant willCleanupNode hook
This commit resolves a memory leak (#11501) in apps running Glimmer. The
root cause of the issue was that a flag governing whether
components/views remove themselves from their parent was being set
incorrectly, so that when the code to cleanup a destroyed view ran, it
was not removed from its parent’s `childViews` array.

Specifically, three hooks are invoked when a render node is cleared:

  1. `env.hooks.willCleanupNode`
  2. `Morph#cleanup`
  3. `env.hooks.didCleanupNode`

Prior to this commit, `willCleanupNode` would blindly set the owner
view’s `isDestroyingSubtree` flag if there was a view set in the `env`
(i.e., basically always).

Sidebar on the `isDestroyingSubtree` flag: this flag is used as a
performance optimization. If a view is destroyed, we want to remove that
view from its parent’s `childViews` array so that garbage collection can
happen. However, it is unnecessary to remove child views from the
destroyed view’s `childViews` array; the GC will take care of any clean
up when the link between the view hierarchy and the destroyed view is
severed.

For example, imagine this hypothetical view hierarchy:

      A
     / \
    B   C
       / \
      D   E

If the render node for view C is destroyed, we need to remove C from
A’s array of child views. However, it is unnecessary to remove D or E
from C’s child views, because they will be imminently removed by the GC,
and temporarily lingering in the child views array does no harm.

We accomplish this by setting the `isDestroyingSubtree` flag on the
root-most view in a hierarchy (view A in the example above), which we
call the owner view. When the render node for C is destroyed, it removes
C from A’s child views array and sets the flag to true. When D and E’s
render nodes are destroyed, they see that the flag has already been
flipped and do not remove their associated views from C’s child views
array.

The memory leak manifested itself when the a render node that did not
have a view associated with it was destroyed, and contained a child
render node that _did_ have a view associated. For example:

```handlebars
{{#if foo}}
  {{my-component}}
{{/if}}
```

In this case, the `willCleanupNode` hook would erroneously set the flag
without actually moving the `my-component` view, because the render node
for the `{{if}}` does not know about the component.

When the cleanup for `{{my-component}}` finally happens (in
Morph#cleanup), the flag has already been set, so the render node
incorrectly assumes that its view is part of a subgraph that has already
been severed.

As far as we can tell, the `willCleanupNode` hook is not doing any
cleanup that is not already done in `Morph#cleanup`. We still do need
`didCleanupNode` to clear the flag, but can leave individual render node
cleanup to the render node itself.

@tomdale tomdale force-pushed the stefanpenner-memory-leak branch to 0d43d24 Jul 7, 2015

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

i'll be testing this shortly

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

I have verified an additional dominator, retaining a similar set of info. Although this PR appears to have fixed something. There exists at-least an additional issue. I'll reduce another example

Tomhuda Katzdale and others added some commits Jul 6, 2015

Eliminate redundant willCleanupNode hook
This commit resolves a memory leak (#11501) in apps running Glimmer. The
root cause of the issue was that a flag governing whether
components/views remove themselves from their parent was being set
incorrectly, so that when the code to cleanup a destroyed view ran, it
was not removed from its parent’s `childViews` array.

Specifically, three hooks are invoked when a render node is cleared:

  1. `env.hooks.willCleanupNode`
  2. `Morph#cleanup`
  3. `env.hooks.didCleanupNode`

Prior to this commit, `willCleanupNode` would blindly set the owner
view’s `isDestroyingSubtree` flag if there was a view set in the `env`
(i.e., basically always).

Sidebar on the `isDestroyingSubtree` flag: this flag is used as a
performance optimization. If a view is destroyed, we want to remove that
view from its parent’s `childViews` array so that garbage collection can
happen. However, it is unnecessary to remove child views from the
destroyed view’s `childViews` array; the GC will take care of any clean
up when the link between the view hierarchy and the destroyed view is
severed.

For example, imagine this hypothetical view hierarchy:

      A
     / \
    B   C
       / \
      D   E

If the render node for view C is destroyed, we need to remove C from
A’s array of child views. However, it is unnecessary to remove D or E
from C’s child views, because they will be imminently removed by the GC,
and temporarily lingering in the child views array does no harm.

We accomplish this by setting the `isDestroyingSubtree` flag on the
root-most view in a hierarchy (view A in the example above), which we
call the owner view. When the render node for C is destroyed, it removes
C from A’s child views array and sets the flag to true. When D and E’s
render nodes are destroyed, they see that the flag has already been
flipped and do not remove their associated views from C’s child views
array.

The memory leak manifested itself when the a render node that did not
have a view associated with it was destroyed, and contained a child
render node that _did_ have a view associated. For example:

```handlebars
{{#if foo}}
  {{my-component}}
{{/if}}
```

In this case, the `willCleanupNode` hook would erroneously set the flag
without actually moving the `my-component` view, because the render node
for the `{{if}}` does not know about the component.

When the cleanup for `{{my-component}}` finally happens (in
Morph#cleanup), the flag has already been set, so the render node
incorrectly assumes that its view is part of a subgraph that has already
been severed.

As far as we can tell, the `willCleanupNode` hook is not doing any
cleanup that is not already done in `Morph#cleanup`. We still do need
`didCleanupNode` to clear the flag, but can leave individual render node
cleanup to the render node itself.
@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

@tomdale I have added an additional failing test.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2015

@stefanpenner Awesome, thank you. We'll take a look at this first thing tomorrow.

@jmurphyau

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2015

@tomdale your PRs are often very detailed.. It's like a blog post around a particular issue/feature.. It's a good read

tomdale and others added some commits Jul 7, 2015

Track parent view of cleaned up render nodes
This commit fixes a bug where views created inside an `{{#each}}` were
not getting removed appropriately from their parent’s `childViews`
array when they were destroyed.

Previously, we used a boolean flag to track whether we had removed the
destroyed view from its parent. The first time we saw a view while
walking the tree of destroyed render nodes, we should remove it from
its parent view’s `childViews` and flip the flag to true.

As we walked the tree, we assumed that any subsequent views were
descendants of the root destroyed view, and therefore could be left in
place while we waited for the GC to do its magic.

However, the case we missed was when there were two sibling views being
destroyed as part of the same root render node being cleaned up.

For example, imagine this view graph:

       A
      / \
     B  /\
       C  D <— both created via an {{#each}}

If the sibling views were inside a non-view-associated render node,
and that render node was cleaned up (e.g., an {{#if}} that became
falsy), the tree of nodes would be walked. Once it reached the render
node associated with the C view, it would remove C from A’s
`childViews` and toggle the flag to true, indicating that we had seen
the view and removed it.

However, the tree walker would then get to the render node
representing the D view. Because the flag was now set to true, it
would erroneously assume D was a child of C and leave it in A’s
`childViews`.

In this commit, we change the flag to a property that tracks the
nearest view to the render node being cleaned up. As we traverse the
render node tree, if we find a node with a view, we remove that view
from it’s parent view’s `childViews` array if and only if its parent
view is the same as the render node’s nearest view. This is a more
correct version of the heuristic for determining when to remove
destroyed views from their parents’ than our previous attempt.
Clarify scope.view / scope.locals.view / env.view
In general, if you want to get access to the “nearest view”, you should
always use `env.view`.

However, some legacy semantics around controllers were a bit quirky.

In some cases, the controller was looked up dynamically from the current
view, but in other cases, the controller was expected to be provided in
another way.

Coincidentally, `scope.view` was only set when the controller was
expected to be derived dynamically from the current view. As a result,
some code was written to check for `scope.view`, and only *then*, look
up its `context` to get the current controller.

Over time, more code came to use `scope.view` when that code actually
wanted to look up the “ambient view”, regardless of whether the ambient
view provided the current controller.

Using `scope.view` in this way had a bad side effect: because it was
sometimes not present (but still used to wire up the view hierarchy in
some cases), the view hierarchy could have “holes”. This manifested
itself as views deep in the hierarchy with `ownerView` set to themselves
and `parentView` set to null.

Not surprisingly, this produced a number of pernicious bugs. The reason
that it was difficult to track this down is that the primary source of
this bug was views at the top level of an outlet. Ember users are very
unlikely to ask for the `parentView` of these views, so they would not
notice the problem.

This would be likely to manifest itself as memory leaks and multiple
root “revalidations” per run loop, rather than a single top-level
revalidation. Both of those behaviors have been observed recently.

This commit removes `scope.view` entirely, and relies on
`scope.locals.view` in the two remaining cases that legitimately needed
this information. All other `scope.view`s were incorrect and have been
changed to `env.view`.

Note that there were some historic bugs in the propagation of `env.view`
that likely caused us (and others) to use `scope.view` in places where
`env.view` would have been more appropriate.
@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2015

@jmurphyau That's why I only get 3-4 PRs done per year.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

let me confirm with the original leak example.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

Still have a leak. Going to prepare another test after dinner.

Good news, the leak appears to be fixed. My apologies for the false alarm, the server (my server) i was serving from has extremely aggressive cache rules. After being unable to create a different failure scenario then the tests I already provided, I double checked the source and realized I was working with stale data.

stefanpenner added a commit that referenced this pull request Jul 8, 2015

Merge pull request #11667 from emberjs/stefanpenner-memory-leak
[BUGFIX release] Fix Glimmer memory leak

@stefanpenner stefanpenner merged commit dc0938c into master Jul 8, 2015

1 check passed

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

@stefanpenner stefanpenner deleted the stefanpenner-memory-leak branch Jul 8, 2015

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

memory leak Monday comes to an end.

Until next week, Same Bat time, same Bat channel!

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

friend reminder @rwjblue this is a bugfix release.

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

@wycats / @tomdale - Please make sure to prefix each commit with '[BUGFIX ]' in the future.

@raytiley

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2015

Sweet!

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jul 8, 2015

@rwjblue Apologies, I was going to rebase and squash into a single commit but got lazy. I will try to remember to do this in the future.

@chriscareycode

This comment has been minimized.

Copy link

commented Jul 8, 2015

Thank you for your hard work on this! Will this land in 1.13.x , or only in 2.x?

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jul 8, 2015

1.13

@jakesjews

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2015

Any chance of this landing in the stable branch soon? This is the only blocker preventing us from merging our conversion to 1.13.

@mixonic

This comment has been minimized.

Copy link
Member

commented Jul 9, 2015

@jakesjews I have no doubt they are working as quickly as possible. Thanks for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.