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

dom-if memory leak #5250

Closed
1 of 6 tasks
madiganz opened this issue Jun 7, 2018 · 19 comments · Fixed by #5508
Closed
1 of 6 tasks

dom-if memory leak #5250

madiganz opened this issue Jun 7, 2018 · 19 comments · Fixed by #5508

Comments

@madiganz
Copy link

madiganz commented Jun 7, 2018

Description

dom-if with restamp causes memory to be leaked until the window is refreshed. This does not work well with SPAs.

Expected outcome

Memory to be cleaned up when the garbage collector runs after a page has been left.

Actual outcome

Memory is leaked until the page is refreshed

Steps to reproduce

Use the polymer-2 starter kit. Modify my-app.html by replacing the normal view code in iron-pages with

<template is="dom-if" if="[[isView1]]" restamp>
    <my-view1 name="view1"></my-view1>
</template>
<template is="dom-if" if="[[isView2]]" restamp>
    <my-view2 name="view2"></my-view2>
</template>
<template is="dom-if" if="[[isView3]]" restamp>
    <my-view3 name="view3"></my-view3>
</template>

Inside of _routePageChanged(page): add this code after this.page = page || 'view';

if (page === 'view1') {
    this.isView1 = true;
    this.isView2 = false;
    this.isView3 = false;
} else if (page === 'view2') {
    this.isView1 = false;
    this.isView2 = true;
    this.isView3 = false;
} else if (page === 'view3') {
    this.isView1 = false;
    this.isView2 = false;
    this.isView3 = true;
}

Adding this code to my-view2.html makes it easy to see the memory leak.

connectedCallback() {
    super.connectedCallback();
    this.hugeArray = [];
    for (var i = 0; i < 1000 * 100; i++) {
        this.hugeArray.push(Math.ceil(Math.random() * 100))
    }
}

This was taken after switching between the views and forcing a garbage collect after landing back on view1.
image

Browsers Affected

  • Chrome
  • Edge
  • Firefox
  • IE 11
  • Safari 8
  • Safari 9

Versions

  • Polymer: v2.6, v3.0.2
  • webcomponents: v1.2
@TimvdLippe
Copy link
Contributor

@madiganz Could you create a small JSBin that I can use to diagnose the issue?

@madiganz
Copy link
Author

madiganz commented Jun 18, 2018

https://jsbin.com/kumoxod/edit?html,console,output

This is my attempt to reproduce this in a JSBin, but it only seems to leak nodes and not any memory. The starter-kits that I used were pretty easy to reproduce the problem. Not sure if this has anything to do with chrome updates, but I never used to see Detached nodes when taking heap snapshots, but now I see them all the time.

I am having a hard replicating the issue in JSBin, but in my opinion, it is very easy to replicate using the starter kit. One thing to note, is if the nodes are being restamped have a dom-if in the view, then there is a memory and node leak, without the dom-if, there is just a node leak.

@madiganz
Copy link
Author

Not sure if this is as helpful, but I created a plunker to show the problem. https://plnkr.co/edit/fes5wfxP1ylWFDFH5kjz?p=preview
Hopefully this helps.

@moezzie
Copy link

moezzie commented Jun 29, 2018

Thanks for reporting this @madiganz.

I believe we're seeing the same issue in our SPA.
It is still using 1.x, but I've been experimenting with 2.x and 3.x which seem to exhibit the same behavior.

@jogibear9988
Copy link

@TimvdLippe any news to this?

I've also problems in my app that controls with dom-if are keept in memory:
bildschirmfoto 2018-07-27 um 21 12 16

@jogibear9988
Copy link

I've found out:

in flush.html enqueueDebouncer is called from dom-if, but flush or flushDebouncers is never called.

see:
bildschirmfoto 2018-07-27 um 21 34 44

@Soul-Master
Copy link

I found this issue on Polymer 2 and it still exists on Polymer 3. Both dom-if and dom-repeat are registered their function to global debouncer.

__debounceRender(fn, delay = 0) {
    this.__renderDebouncer = Debouncer.debounce(
          this.__renderDebouncer
        , delay > 0 ? timeOut.after(delay) : microTask
        , fn.bind(this));
    enqueueDebouncer(this.__renderDebouncer);
  }

Items in this global debouncer list cannot be removed. It causes memory leak for any elements that use this elements in their template. We need to manually execute the following statement to prevent this memory leak.

Polymer.flush();

@madiganz
Copy link
Author

I added Polymer.flush() to _routePageChanged(page) in my-app.html in the polymer 2 starter kit. This is my result from the same tests as before.

image

While it seems like with this change, it is no longer leaking memory, it is still leaking nodes.

image

@200Puls
Copy link

200Puls commented Feb 5, 2019

I'm using Polymer 2.5. and just encountered the same problem. The memory consumption is growing and growing until the page is refreshed. With a failry large application it doesnt take long for the app to become very unresponsive.

@davidsteinberger
Copy link

I'm seeing the same. Is there any safe way to cleanup all those detached instances?

@kevinpschaaf
Copy link
Member

Confirmed this issue. Fix will be in #5499 & #5500. As a short-term workaround, periodically calling flush() from lib/utils/flush.js (Polymer.flush() in 2.x) will free the detached instances for gc.

@davidsteinberger
Copy link

@kevinpschaaf thanks! I'm wondering though if that truly fixes the mem-leak. I monkey-patched a similar fix into our app and while the memory consumption decreased after a forced gc I kept getting more and more detached nodes. Will test soon.

kevinpschaaf added a commit that referenced this issue Feb 28, 2019
[3.x] Use set and clear debouncer upon completion. Fixes #5250.
@kevinpschaaf
Copy link
Member

@davidsteinberger On a local test designed to stress this specific memory leak, I confirmed this results in a flat memory profile vs. the previous release. If you can repro a leak (same or otherwise), please let us know.

@Soul-Master
Copy link

@kevinpschaaf
When do you will release new version of Polymer 3 that includes this fix? Last version of Polymer 3 was released more than 5 months ago.

@davidsteinberger
Copy link

davidsteinberger commented Mar 4, 2019

@kevinpschaaf why was that reverted on https://github.com/Polymer/polymer/tree/2.x?
A new release of Polymer 2 would also be great.

@kevinpschaaf
Copy link
Member

@Soul-Master Yes, we'll get a new release of both 2.x and 3.x out, hopefully in the next couple of days.

@davidsteinberger There was an unintentional push to the 2.x branch there 🙄; the PR to the 2.x branch is in #5500 and will be merged shortly pending some last internal testing. We'll release both 2.x and 3.x together with this fix.

@madiganz
Copy link
Author

@kevinpschaaf Thank you for working on this! I am sure we are all happy to have this fixed. Any updates on when this will be released for 2.x?

kevinpschaaf added a commit that referenced this issue Mar 20, 2019
[2.x] Use set and clear debouncer upon completion. Fixes #5250
@nazarsa
Copy link

nazarsa commented Sep 3, 2019

I'm using Polymer 2.5. and just encountered the same problem. The memory consumption is growing and growing until the page is refreshed. With a failry large application it doesnt take long for the app to become very unresponsive.

Does refresh clear the memory leak in 2.5 or ONLY with 3.0?

@madiganz
Copy link
Author

madiganz commented Sep 3, 2019

@nazarsa For Polymer 2, the fix for this is in 2.7.1.

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

Successfully merging a pull request may close this issue.

9 participants