Lit-element: potential memory leak

[rudzikdawid]

Description

Live Demo

example of pagination:
https://stackblitz.com/edit/lit-element-1zn35m code
https://lit-element-1zn35m.stackblitz.io/ demo

example of changing view on interval:
https://stackblitz.com/edit/lit-element-amucfu code
https://lit-element-amucfu.stackblitz.io demo

Steps to Reproduce

1. open https://stackblitz.com/edit/lit-element-1zn35m
2. click the > (next page) button in the bottom right corner
3. each pagination change creates 8KB of garbage

Intensive page changing creates memory leak

Expected Results

no memory leak

Actual Results

memory leak

Browsers Affected

• [x] Chrome
• [ ] Firefox
• [ ] Edge
• [ ] Safari 11
• [ ] Safari 10
• [ ] IE 11

Versions

• lit-element: 2.2.1

[bengfarrell]

I'm newish to LitElement, so take what I say with a grain of salt.

What I think is happening here, is that LitElement is re-using those same button instances, rather than removing all six, just to then re-instance 3 right away.

Add a console log on the connected callback as well, you'll see that likewise, you're only getting 3 connectedCallbacks when using the back button, despite there being 6 buttons on that first page.

LitElement (actually lit-html under the hood), does some pretty decent dom diffing, such that in this case, as the elements themselves don't change, there's no reason to tear EVERYTHING down. Only the contents of those elements change, so it does that instead.

[rudzikdawid]

@bengfarrell you have an idea where the memory leak comes from ?

[bengfarrell]

@rudzikdawid Sorry, I guess I wasn't clear - I'm suggesting that there is no memory leak in this case. In a nutshell, you're starting with 6 buttons, and you're removing 3 (regardless of what content is in them). Therefore, 3 disconnectedCallbacks would be expected here instead of 6.

[justinfagnani]

@bengfarrell is correct here. The array passed here:

      <div class="buttons-container">
        ${this.buttonsOnCurrentPage.length ? this.buttonsOnCurrentPage.map(item => html`...

contains items with the same template, so they're simply updated in place. The three logs you see when going to page 2 are because the list on page 2 is three shorter. This is also why you don't see any logs when going from page 1 to page 2.

Closing as working as intended.

[rudzikdawid]

@justinfagnani You have an idea why it's leaking? each pagination change leaves 8KB of garbage.

[justinfagnani]

The 10KB of allocation aren't too concerning on their own, but digging into overall memory, nodes, and event listeners I do see a leak. It appears that the document retains references to ShadowRoots when using adoptedStyleSheets: https://bugs.chromium.org/p/chromium/issues/detail?id=1002763

[rudzikdawid]

@justinfagnani thank You. i moved styles from static get styles() {...} to <style></style> element inside render() method. Memory leak has disappeared.

So mayby in guide https://lit-element.polymer-project.org/guide/styles#static-styles should be warn about memory leak in chromium-browser? Until this problem will be resolved?
Now we have information that is not quite true:

We recommend using static styles for optimal performance.

I agree that in application which is running a maximum of several minutes, 10 KB memory leak on each view change does not look spectacular.

But in a dashboard application based on websockets that should work for several hours and even more. 10KB on each small view change is unacceptable. There are many scenarios where the application must run constantly. A small leak in that case can accumulate hundreds of megabytes per day.

[justinfagnani]

We don't want users changing their element code for a bug. At worst we would disable using native constructible stylesheets until the bug is fixed, which would require no code changes from users, but would require updating lit-element. Another option is to include:

<script>
  // remove when https://bugs.chromium.org/p/chromium/issues/detail?id=1002763 is fixed
  delete Document.prototype.adoptedStyleSheets;
</script>

in the page to defeat LitElement's feature detection.

I want to see the response to the bug before recommending any changes though.

cc @rakina

[jsilvermist]

https://bugs.chromium.org/p/chromium/issues/detail?id=1002763

Anything public? All I get is Permission denied.

[justinfagnani]

@jsilvermist sorry, it defaulted to private for my account. I need to ask if I can flip it public. I don't work in Chromium issues enough to know what the policy is, but I'm sure it's ok.

Good news though, a fix is up already: https://chromium-review.googlesource.com/c/chromium/src/+/1798222

[jsilvermist]

@justinfagnani Awesome, thanks!

[justinfagnani]

The Chrome memory leak fix will ship with 78. I don't think there's anything more to do on our end.