fix(grid): plug a memory leak from the render process #3721
Conversation
Westbrook
requested review from
moisadoru,
Rajdeepc,
blunteshwar,
najikahalsema and
TarunAdobe
Tachometer resultsChromegrid permalink
Firefoxgrid permalink
|
| expect( | ||
| afterMB - beforeMB, | ||
| `before: ${beforeMB}, after: ${afterMB}` | ||
| ).to.be.lt(0); |
There was a problem hiding this comment.
You can also potentially use a WeakRef to the Grid element and wait a certain amount of time/ trigger garbage collection and expect the deref() to return undefined. This might be more deterministic than measuring heap memory.
There was a problem hiding this comment.
Opted for the more reliable, but slightly more cumbersome to surface, [performance.measureUserAgentSpecificMemory()](https://chromestatus.com/feature/5685965186138112) for now. Showing even more positive results from this change when measuring with this API.
I know the team has been looking at tooling with WeakMaps, I wonder how you're actually holding things in that way as it feels like the need for the key would inherently keep a reference and prevent the memory from being released. You wouldn't happen to have any examples to share?
There was a problem hiding this comment.
We're mainly using WeakRef different from WeakMap. The basic idea is to construct a new WeakRef passing in the DOM node you're interested in tracking, trigger garbage collection, and then test whether or not calling WeakRef.deref() on your reference returns undefined. If so, you can assume the node was garbage collected. It not fool-proof but it has been working quite nicely for us.
Here is the original dom inspection snippets that Kin created: https://git.corp.adobe.com/jblas/debug-utils/tree/main/snippets#object-watcher
There was a problem hiding this comment.
This is cool, I didn't realize that WeakRef was different than WeakMap! Definitely a useful pattern for the future, particularly as a comparison for test speed for tightening the feedback cycle.
Digging into measureUserAgentSpecificMemory() will also give us some other useful tidbits later on, for an expanded perf testing suite, so getting started now seems useful.
Westbrook
force-pushed
the
grid-leak
branch
2 times, most recently
from
675ccee
to
f0c3d7b
Compare
Westbrook
force-pushed
the
grid-leak
branch
4 times, most recently
from
a4fe469
to
bb55ba0
Compare
| @@ -135,5 +143,5 @@ export default { | |||
| }, | |||
| ], | |||
| group: 'unit', | |||
| browsers: [chromium, firefox, webkit], | |||
| browsers: [chromiumWithMemoryTooling, firefox, webkit], | |||
There was a problem hiding this comment.
is it safe to run this "flag-customized" version of chromium for all tests? or should you create a group config using it that focuses only on the grid memory tests?
There was a problem hiding this comment.
In that the tests run and pass, I have no other path to judge "safety" here.
This is a great point though, and segmenting these out, for safety/performance/understandability/etc, as part of #3722 would make a lot of sense!
Westbrook
force-pushed
the
grid-leak
branch
2 times, most recently
from
4998049
to
fde6e76
Compare
Westbrook
force-pushed
the
grid-leak
branch
3 times, most recently
from
12a1099
to
d01ec57
Compare
Rajdeepc
changed the title
Description
Rendering a
lit-htmltemplate withrender()without caching it's root part means that you can't inform it when the template is connected to the DOM or not. This connection state is required to ensure clean up can happen to any of the template directives that might be leveraged therein. If the Virtualize template directive does not get disconnected it can leak references to the items it had previously rendered to the DOM.Cache a reference to the "grid part" in the Grid element for the DOM that would otherwise have been rendered to the render root and managed directly by
LitElement. Manage the connected state of this cached part.Related issue(s)
How has this been tested?
Types of changes
Checklist