Fix a memory leak with the shared TreeWalker by justinfagnani · Pull Request #3888 · lit/lit

justinfagnani · 2023-05-09T16:45:50Z

We weren't resetting the currentNode of the shared TreeWalker after either template prep or template cloning. This caused the TreeWalker to hold on to the last currentNode of the last tree walk.

This should always be after a clone because I can't think of a situation where we prep a template but don't immediately render it. So for the fix I set currentNode after a template clone. You can't set TreeWalker.currentNode to null, so I set it to the document.

changeset-bot · 2023-05-09T16:45:53Z

🦋 Changeset detected

Latest commit: e112cf3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
lit-html	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2023-05-09T16:46:11Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -2% - +8% (-0.42ms - +1.50ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 89.72ms - 93.45ms
lit-html-kitchen-sink: unsure 🔍 -2% - +10% (-0.65ms - +3.56ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -11% - +9% (-1.40ms - +1.18ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +2% (-2.09ms - +1.57ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -3% - +4% (-1.75ms - +2.34ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 921.82ms - 931.64ms
lit-html-kitchen-sink: unsure 🔍 -5% - +3% (-5.16ms - +3.04ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -9% - +15% (-32.95ms - +52.38ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +1% (-3.56ms - +1.71ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-7.12ms - +11.25ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 886.41ms - 896.97ms
reactive-element-list: unsure 🔍 -1% - +1% (-12.10ms - +6.43ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 112.0.5615.165
Sample size: 50
Built by: Benchmarks #4756
Commit: 7e3b491

render

Version	Avg time	vs
	89.72ms - 93.45ms	-

update

Version	Avg time	vs
	921.82ms - 931.64ms	-

update-reflect

Version	Avg time	vs
	886.41ms - 896.97ms	-

lit-html-kitchen-sink

Browser: chrome-headless 112.0.5615.165
Sample size: 30
Built by: Benchmarks #4756
Commit: 7e3b491

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	35.31ms - 38.44ms	-	unsure 🔍 -2% - +10% -0.65ms - +3.56ms	unsure 🔍 -5% - +8% -1.95ms - +2.87ms
tip-of-tree tip-of-tree	34.01ms - 36.83ms	unsure 🔍 -10% - +2% -3.56ms - +0.65ms	-	unsure 🔍 -9% - +4% -3.31ms - +1.32ms
previous-release previous-release	34.58ms - 38.25ms	unsure 🔍 -8% - +5% -2.87ms - +1.95ms	unsure 🔍 -4% - +9% -1.32ms - +3.31ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	89.26ms - 95.76ms	-	unsure 🔍 -5% - +3% -5.16ms - +3.04ms	unsure 🔍 -7% - +2% -6.90ms - +1.92ms
tip-of-tree tip-of-tree	91.07ms - 96.06ms	unsure 🔍 -3% - +6% -3.04ms - +5.16ms	-	unsure 🔍 -6% - +3% -5.32ms - +2.46ms
previous-release previous-release	92.02ms - 97.98ms	unsure 🔍 -2% - +8% -1.92ms - +6.90ms	unsure 🔍 -3% - +6% -2.46ms - +5.32ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	19.43ms - 20.70ms	-	unsure 🔍 -2% - +8% -0.42ms - +1.50ms	unsure 🔍 -2% - +6% -0.41ms - +1.13ms
tip-of-tree tip-of-tree	18.81ms - 20.25ms	unsure 🔍 -7% - +2% -1.50ms - +0.42ms	-	unsure 🔍 -5% - +3% -1.02ms - +0.66ms
previous-release previous-release	19.27ms - 20.14ms	unsure 🔍 -6% - +2% -1.13ms - +0.41ms	unsure 🔍 -3% - +5% -0.66ms - +1.02ms	-

lit-html-repeat

Browser: chrome-headless 112.0.5615.165
Sample size: 30
Built by: Benchmarks #4756
Commit: 7e3b491

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.16ms - 13.48ms	-	unsure 🔍 -11% - +9% -1.40ms - +1.18ms	unsure 🔍 -6% - +8% -0.70ms - +1.03ms
tip-of-tree tip-of-tree	11.82ms - 14.04ms	unsure 🔍 -9% - +11% -1.18ms - +1.40ms	-	unsure 🔍 -8% - +12% -0.96ms - +1.51ms
previous-release previous-release	12.10ms - 13.21ms	unsure 🔍 -8% - +5% -1.03ms - +0.70ms	unsure 🔍 -12% - +7% -1.51ms - +0.96ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	339.68ms - 401.45ms	-	unsure 🔍 -9% - +15% -32.95ms - +52.38ms	unsure 🔍 -12% - +10% -46.41ms - +37.71ms
tip-of-tree tip-of-tree	331.41ms - 390.28ms	unsure 🔍 -14% - +9% -52.38ms - +32.95ms	-	unsure 🔍 -14% - +7% -55.07ms - +26.93ms
previous-release previous-release	346.37ms - 403.46ms	unsure 🔍 -10% - +13% -37.71ms - +46.41ms	unsure 🔍 -8% - +15% -26.93ms - +55.07ms	-

lit-html-template-heavy

Browser: chrome-headless 112.0.5615.165
Sample size: 50
Built by: Benchmarks #4756
Commit: 7e3b491

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	65.66ms - 68.30ms	-	unsure 🔍 -3% - +2% -2.09ms - +1.57ms	unsure 🔍 -2% - +3% -1.50ms - +2.14ms
tip-of-tree tip-of-tree	65.98ms - 68.51ms	unsure 🔍 -2% - +3% -1.57ms - +2.09ms	-	unsure 🔍 -2% - +4% -1.20ms - +2.37ms
previous-release previous-release	65.41ms - 67.92ms	unsure 🔍 -3% - +2% -2.14ms - +1.50ms	unsure 🔍 -4% - +2% -2.37ms - +1.20ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	136.64ms - 140.28ms	-	unsure 🔍 -3% - +1% -3.56ms - +1.71ms	unsure 🔍 -3% - +1% -3.75ms - +1.58ms
tip-of-tree tip-of-tree	137.48ms - 141.30ms	unsure 🔍 -1% - +3% -1.71ms - +3.56ms	-	unsure 🔍 -2% - +2% -2.89ms - +2.57ms
previous-release previous-release	137.60ms - 141.49ms	unsure 🔍 -1% - +3% -1.58ms - +3.75ms	unsure 🔍 -2% - +2% -2.57ms - +2.89ms	-

reactive-element-list

Browser: chrome-headless 112.0.5615.165
Sample size: 50
Built by: Benchmarks #4756
Commit: 7e3b491

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	57.67ms - 60.67ms	-	unsure 🔍 -3% - +4% -1.75ms - +2.34ms	unsure 🔍 -7% - +0% -4.11ms - +0.19ms
tip-of-tree tip-of-tree	57.49ms - 60.26ms	unsure 🔍 -4% - +3% -2.34ms - +1.75ms	-	faster ✔ 0% - 7% 0.19ms - 4.33ms
previous-release previous-release	59.59ms - 62.67ms	unsure 🔍 -0% - +7% -0.19ms - +4.11ms	slower ❌ 0% - 7% 0.19ms - 4.33ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	961.76ms - 974.26ms	-	unsure 🔍 -1% - +1% -7.12ms - +11.25ms	unsure 🔍 -1% - +1% -9.50ms - +9.88ms
tip-of-tree tip-of-tree	959.21ms - 972.67ms	unsure 🔍 -1% - +1% -11.25ms - +7.12ms	-	unsure 🔍 -1% - +1% -11.88ms - +8.13ms
previous-release previous-release	960.42ms - 975.22ms	unsure 🔍 -1% - +1% -9.88ms - +9.50ms	unsure 🔍 -1% - +1% -8.13ms - +11.88ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	943.02ms - 956.62ms	-	unsure 🔍 -1% - +1% -12.10ms - +6.43ms	unsure 🔍 -1% - +1% -11.04ms - +9.13ms
tip-of-tree tip-of-tree	946.35ms - 958.96ms	unsure 🔍 -1% - +1% -6.43ms - +12.10ms	-	unsure 🔍 -1% - +1% -7.88ms - +11.64ms
previous-release previous-release	943.32ms - 958.23ms	unsure 🔍 -1% - +1% -9.13ms - +11.04ms	unsure 🔍 -1% - +1% -11.64ms - +7.88ms	-

_{tachometer-reporter-action v2 for Benchmarks}

rictic · 2023-05-09T17:27:30Z

Nice!

justinfagnani added 2 commits May 9, 2023 09:34

Fix a memory leak with the shared TreeWalker

4d091e6

Use document as the parking node

e112cf3

justinfagnani requested a review from rictic May 9, 2023 16:45

justinfagnani requested a review from kevinpschaaf as a code owner May 9, 2023 16:45

rictic approved these changes May 9, 2023

View reviewed changes

justinfagnani merged commit 0f30e6f into main May 9, 2023

justinfagnani deleted the walker-leak branch May 9, 2023 17:30

lit-robot mentioned this pull request May 11, 2023

Version Packages #3898

Merged

AndrewJakubowicz mentioned this pull request May 15, 2023

[lit-html] Memory/Handlers Leak Issues: Rendering Tree of CustomElement using only lit-html, Regression? #3065

Closed

Conversation

justinfagnani commented May 9, 2023

Uh oh!

changeset-bot bot commented May 9, 2023

🦋 Changeset detected

Uh oh!

github-actions bot commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

Uh oh!

rictic commented May 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented May 9, 2023 •

edited

Loading