[labs/ssr] fix patched directives memory leak by AndrewJakubowicz · Pull Request #4515 · lit/lit

AndrewJakubowicz · 2024-01-23T23:46:48Z

Context

A memory leak was reported in Lit SSR.

Found by testing with #4514, and going test by test.

The repeat directive test contained this leak. After this PR the directive test no longer leaks.

So far no other test shows memory leak characteristics.

Why?

Because we have a cache of WeakMap<non patched directive, patched directive>. This is an issue because we can pull a patched directive off the value, and then it won't be found in the WeakMap. Because it's in the value of the WeakMap and not the key. Thus the patched directive gets patched again and stored in the WeakMap. This repeats for the number of patch calls resulting in a nested doll of patched directive constructors.

Fix

I implemented a super simple naive fix. Just flag the patched directive ctor with a sentinel property. Now when pulling the directive constructor off the value, we can immediately check if it's patched by looking at the sentinel.

Fix was updated per Justin's feedback. It now patches the _$resolve field. This simplifies a lot of the book keeping code. Which has a much nicer memory footprint.

However, this fix does need to be backwards compatible hence the new API, patchDirectiveResolve. This allows for the following:

User has lit-html and ssr, and bumps lit-html only. The older ssr package can still use overrideDirectiveResolve.
User has lit-html and ssr, and bumps ssr. This will need to depend on the version of lit-html that introduces patchDirectiveResolve, v3.1.2 (unreleased).

Test plan

No memory test for this change because memory tests are expensive and slow. Instead this is covered by existing behavior tests. I can add tests if required.
The memory characteristics of this change were verified by pulling this change into #4514

Plan is to commit a memory test as a separate PR.

Each time a patched directive is encountered, it gets patched again creating a subtle memory leak

changeset-bot · 2024-01-23T23:46:52Z

🦋 Changeset detected

Latest commit: 7e4bd67

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

This PR includes changesets to release 2 packages

Name	Type
@lit-labs/ssr	Patch
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 · 2024-01-23T23:47:02Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +5% (-0.54ms - +0.58ms)
^{this-change vs tip-of-tree}

render

this-change: 49.13ms - 51.66ms
this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +3% (-0.94ms - +0.60ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +3% (-0.87ms - +0.87ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: faster ✔ 0% - 4% (0.02ms - 1.23ms)
^{this-change vs tip-of-tree}

update

this-change: 521.21ms - 531.93ms
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +9% (-1.28ms - +3.41ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +3% (-0.29ms - +1.78ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +0% (-2.88ms - +1.43ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 523.29ms - 531.28ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +0% (-3.70ms - +2.15ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 120.0.6099.224
Sample size: 50
Built by: Benchmarks #6070
Commit: 7e4bd67

render

Version	Avg time	vs
	49.13ms - 51.66ms	-

update

Version	Avg time	vs
	521.21ms - 531.93ms	-

update-reflect

Version	Avg time	vs
	523.29ms - 531.28ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 120.0.6099.224
Sample size: 30
Built by: Benchmarks #6070
Commit: 7e4bd67

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.30ms - 19.30ms	-	unsure 🔍 -5% - +3% -0.94ms - +0.60ms	unsure 🔍 -6% - +1% -1.12ms - +0.29ms
tip-of-tree tip-of-tree	18.39ms - 19.55ms	unsure 🔍 -3% - +5% -0.60ms - +0.94ms	-	unsure 🔍 -5% - +3% -1.01ms - +0.51ms
previous-release previous-release	18.73ms - 19.71ms	unsure 🔍 -2% - +6% -0.29ms - +1.12ms	unsure 🔍 -3% - +5% -0.51ms - +1.01ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	39.21ms - 42.63ms	-	unsure 🔍 -3% - +9% -1.28ms - +3.41ms	unsure 🔍 -4% - +8% -1.71ms - +3.29ms
tip-of-tree tip-of-tree	38.26ms - 41.46ms	unsure 🔍 -8% - +3% -3.41ms - +1.28ms	-	unsure 🔍 -7% - +5% -2.70ms - +2.15ms
previous-release previous-release	38.32ms - 41.95ms	unsure 🔍 -8% - +4% -3.29ms - +1.71ms	unsure 🔍 -5% - +7% -2.15ms - +2.70ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.04ms - 11.81ms	-	unsure 🔍 -5% - +5% -0.54ms - +0.58ms	unsure 🔍 -5% - +5% -0.60ms - +0.59ms
tip-of-tree tip-of-tree	11.00ms - 11.80ms	unsure 🔍 -5% - +5% -0.58ms - +0.54ms	-	unsure 🔍 -6% - +5% -0.63ms - +0.58ms
previous-release previous-release	10.98ms - 11.88ms	unsure 🔍 -5% - +5% -0.59ms - +0.60ms	unsure 🔍 -5% - +6% -0.58ms - +0.63ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 120.0.6099.224
Sample size: 50
Built by: Benchmarks #6070
Commit: 7e4bd67

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.47ms - 34.45ms	-	unsure 🔍 -3% - +3% -0.87ms - +0.87ms	unsure 🔍 -2% - +2% -0.82ms - +0.69ms
tip-of-tree tip-of-tree	33.24ms - 34.68ms	unsure 🔍 -3% - +3% -0.87ms - +0.87ms	-	unsure 🔍 -3% - +3% -0.99ms - +0.86ms
previous-release previous-release	33.45ms - 34.61ms	unsure 🔍 -2% - +2% -0.69ms - +0.82ms	unsure 🔍 -3% - +3% -0.86ms - +0.99ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	70.00ms - 71.69ms	-	unsure 🔍 -0% - +3% -0.29ms - +1.78ms	unsure 🔍 -1% - +2% -1.05ms - +1.49ms
tip-of-tree tip-of-tree	69.50ms - 70.70ms	unsure 🔍 -3% - +0% -1.78ms - +0.29ms	-	unsure 🔍 -2% - +1% -1.65ms - +0.60ms
previous-release previous-release	69.67ms - 71.58ms	unsure 🔍 -2% - +1% -1.49ms - +1.05ms	unsure 🔍 -1% - +2% -0.60ms - +1.65ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 120.0.6099.224
Sample size: 50
Built by: Benchmarks #6070
Commit: 7e4bd67

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.37ms - 34.17ms	-	faster ✔ 0% - 4% 0.02ms - 1.23ms	unsure 🔍 -2% - +2% -0.58ms - +0.60ms
tip-of-tree tip-of-tree	33.94ms - 34.84ms	slower ❌ 0% - 4% 0.02ms - 1.23ms	-	slower ❌ 0% - 4% 0.01ms - 1.26ms
previous-release previous-release	33.33ms - 34.19ms	unsure 🔍 -2% - +2% -0.60ms - +0.58ms	faster ✔ 0% - 4% 0.01ms - 1.26ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	532.73ms - 535.42ms	-	unsure 🔍 -1% - +0% -2.88ms - +1.43ms	unsure 🔍 -0% - +0% -1.85ms - +2.29ms
tip-of-tree tip-of-tree	533.12ms - 536.48ms	unsure 🔍 -0% - +1% -1.43ms - +2.88ms	-	unsure 🔍 -0% - +1% -1.35ms - +3.25ms
previous-release previous-release	532.28ms - 535.43ms	unsure 🔍 -0% - +0% -2.29ms - +1.85ms	unsure 🔍 -1% - +0% -3.25ms - +1.35ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	538.75ms - 542.41ms	-	unsure 🔍 -1% - +0% -3.70ms - +2.15ms	unsure 🔍 -1% - +1% -5.80ms - +4.10ms
tip-of-tree tip-of-tree	539.07ms - 543.64ms	unsure 🔍 -0% - +1% -2.15ms - +3.70ms	-	unsure 🔍 -1% - +1% -5.22ms - +5.06ms
previous-release previous-release	536.83ms - 546.04ms	unsure 🔍 -1% - +1% -4.10ms - +5.80ms	unsure 🔍 -1% - +1% -5.06ms - +5.22ms	-

_{tachometer-reporter-action v2 for Benchmarks}

github-actions · 2024-01-23T23:48:49Z

The size of lit-html.js and lit-core.min.js are as expected.

rictic · 2024-01-24T01:45:53Z

Nice!

augustjk

Good find and fix!

I looked into whether it might be better to add the sentinel property in overrideDirectiveResolve but that lives in lit-html exported via private-ssr-support so it seems better to keep everything in this ssr package.

packages/labs/ssr/src/lib/render-value.ts

Note, this has to be done in a way that is backwards compatible. If lit-html is updated, and ssr is not updated, then overrideDirectiveResolve must continue to be accessible. Thus we add a new "public" private ssr api for patching _$resolve.

AndrewJakubowicz · 2024-01-24T22:06:32Z

Great feedback which has been incorporated. Description has also been updated. Thank you!

packages/lit-html/src/private-ssr-support.ts

This reverts commit a1971f1.

packages/labs/ssr/src/lib/render-value.ts

packages/lit-html/src/private-ssr-support.ts

packages/labs/ssr/src/lib/render-value.ts

augustjk

LGTM

packages/lit-html/src/private-ssr-support.ts

justinfagnani

nice work!

packages/lit-html/src/private-ssr-support.ts

prevent patching of patched directives memory leak

feb34fc

Each time a patched directive is encountered, it gets patched again creating a subtle memory leak

AndrewJakubowicz requested a review from kevinpschaaf as a code owner January 23, 2024 23:46

add changeset

2c1e4d6

AndrewJakubowicz requested review from augustjk, justinfagnani and rictic January 24, 2024 00:04

rictic approved these changes Jan 24, 2024

View reviewed changes

augustjk approved these changes Jan 24, 2024

View reviewed changes

justinfagnani reviewed Jan 24, 2024

View reviewed changes

packages/labs/ssr/src/lib/render-value.ts Outdated Show resolved Hide resolved

justinfagnani requested changes Jan 24, 2024

View reviewed changes

packages/labs/ssr/src/lib/render-value.ts Outdated Show resolved Hide resolved

AndrewJakubowicz added 3 commits January 24, 2024 13:52

simplify types

a1971f1

update changeset

4f436bf

AndrewJakubowicz requested a review from justinfagnani January 24, 2024 22:05

recommended by Augustine to ensure ssr depends on the right lit-html

1668ef9

AndrewJakubowicz requested review from augustjk and rictic January 24, 2024 22:06

augustjk reviewed Jan 24, 2024

View reviewed changes

packages/lit-html/src/private-ssr-support.ts Outdated Show resolved Hide resolved

packages/lit-html/src/private-ssr-support.ts Show resolved Hide resolved

packages/lit-html/src/private-ssr-support.ts Outdated Show resolved Hide resolved

Revert "simplify types"

c3a135f

This reverts commit a1971f1.

justinfagnani reviewed Jan 24, 2024

View reviewed changes

packages/labs/ssr/src/lib/render-value.ts Show resolved Hide resolved

justinfagnani reviewed Jan 24, 2024

View reviewed changes

packages/lit-html/src/private-ssr-support.ts Outdated Show resolved Hide resolved

packages/labs/ssr/src/lib/render-value.ts Show resolved Hide resolved

add a comment to note the possibility of a prototype walking solution

051083a

AndrewJakubowicz requested a review from justinfagnani January 25, 2024 21:08

augustjk approved these changes Jan 25, 2024

View reviewed changes

justinfagnani reviewed Jan 27, 2024

View reviewed changes

packages/lit-html/src/private-ssr-support.ts Outdated Show resolved Hide resolved

packages/lit-html/src/private-ssr-support.ts Show resolved Hide resolved

This was referenced Jan 29, 2024

[labs/ssr] Investigate interactions with minified or unminified lit-html #4527

Open

[labs/ssr] fix patching of patched directives memory leak #4528

Closed

Walk the prototype to patch the Directive _$resolve method

28b4170

justinfagnani approved these changes Jan 30, 2024

View reviewed changes

AndrewJakubowicz added 2 commits January 30, 2024 14:35

some code review cleanup

16628f5

update error comment

7e4bd67

AndrewJakubowicz changed the title ~~[lit-labs/ssr] prevent patching of patched directives memory leak~~ [labs/ssr] fix patched directives memory leak Jan 30, 2024

AndrewJakubowicz merged commit dca963f into main Jan 30, 2024

AndrewJakubowicz deleted the fix-ssr-directive-memory-leak branch January 30, 2024 23:09

lit-robot mentioned this pull request Jan 31, 2024

Version Packages #4532

Merged

AndrewJakubowicz mentioned this pull request Feb 1, 2024

[infra] revert changeset config.json #4536

Merged

Conversation

AndrewJakubowicz commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Why?

Fix

Test plan

Uh oh!

changeset-bot bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jan 23, 2024 • 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

update-reflect

Uh oh!

github-actions bot commented Jan 23, 2024

Uh oh!

rictic commented Jan 24, 2024

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewJakubowicz commented Jan 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augustjk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AndrewJakubowicz commented Jan 23, 2024 •

edited

Loading

changeset-bot bot commented Jan 23, 2024 •

edited

Loading

github-actions bot commented Jan 23, 2024 •

edited

Loading