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

[AndrewJakubowicz]

Context

This has been split out of #4515 which explored a performant and more optimal fix. However, this revealed some possible other issues, e.g. #4527.

This change is a super tiny incremental fix which resolves the leak, and leaves the door open for future improvements.

How does this resolve the leak?

The leak occurs due to directive constructors being re-used. However on re-use, the constructor is patched and isn't in the key of the WeakMap. The leak is then caused by patching the directive by applying another patch mixin. This occurs over and over. Because each patched directive extends the prior patched directive, they can never be cleaned up and on each SSR render you end up with directives being extended an additional time.

By adding the patched directive into the key, when the patched directive is encountered, we no longer patch it again. Thus fixing the leak.

Test plan

This was manually tested with #4514

Risk

The risk of this is tiny since the change is laser focused, and results in a patch bump to only SSR (Whilst the solution explored in #4515 requires lit-html to be bumped).

Credit to @augustjk for this much simpler fix that my initial branding of the patched directive.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7918f2f

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

This PR includes changesets to release 1 package

Name	Type
@lit-labs/ssr	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]

📊 Tachometer Benchmark Results

Summary

nop-update

• this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +4% (-0.61ms - +0.43ms)
  ^{this-change vs tip-of-tree}

render

• this-change: 48.77ms - 51.39ms
• this-change, tip-of-tree, previous-release: unsure 🔍 -4% - +3% (-0.71ms - +0.65ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: slower ❌ 0% - 4% (0.14ms - 1.41ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-0.89ms - +0.32ms)
  ^{this-change vs tip-of-tree}

update

• this-change: 530.65ms - 541.78ms
• this-change, tip-of-tree, previous-release: unsure 🔍 -7% - +6% (-2.70ms - +2.34ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +4% (-0.02ms - +2.56ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-1.78ms - +4.73ms)
  ^{this-change vs tip-of-tree}

update-reflect

• this-change: 532.88ms - 539.43ms
• this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.03ms - +3.55ms)
  ^{this-change vs tip-of-tree}

Results

this-change

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6065

• Commit: 7918f2f

render

Version	Avg time	vs
	48.77ms - 51.39ms	-

update

Version	Avg time	vs
	530.65ms - 541.78ms	-

update-reflect

Version	Avg time	vs
	532.88ms - 539.43ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 30
• Built by: Benchmarks #6065

• Commit: 7918f2f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	18.42ms - 19.43ms	-	unsure 🔍 -4% - +3% -0.71ms - +0.65ms	unsure 🔍 -4% - +3% -0.79ms - +0.59ms
tip-of-tree tip-of-tree	18.50ms - 19.42ms	unsure 🔍 -3% - +4% -0.65ms - +0.71ms	-	unsure 🔍 -4% - +3% -0.73ms - +0.59ms
previous-release previous-release	18.55ms - 19.49ms	unsure 🔍 -3% - +4% -0.59ms - +0.79ms	unsure 🔍 -3% - +4% -0.59ms - +0.73ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	38.81ms - 42.46ms	-	unsure 🔍 -7% - +6% -2.70ms - +2.34ms	unsure 🔍 -9% - +3% -3.94ms - +1.27ms
tip-of-tree tip-of-tree	39.07ms - 42.56ms	unsure 🔍 -6% - +7% -2.34ms - +2.70ms	-	unsure 🔍 -9% - +3% -3.71ms - +1.39ms
previous-release previous-release	40.11ms - 43.83ms	unsure 🔍 -3% - +10% -1.27ms - +3.94ms	unsure 🔍 -3% - +9% -1.39ms - +3.71ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.01ms - 11.75ms	-	unsure 🔍 -5% - +4% -0.61ms - +0.43ms	unsure 🔍 -6% - +3% -0.67ms - +0.39ms
tip-of-tree tip-of-tree	11.11ms - 11.83ms	unsure 🔍 -4% - +5% -0.43ms - +0.61ms	-	unsure 🔍 -5% - +4% -0.57ms - +0.47ms
previous-release previous-release	11.14ms - 11.89ms	unsure 🔍 -3% - +6% -0.39ms - +0.67ms	unsure 🔍 -4% - +5% -0.47ms - +0.57ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6065

• Commit: 7918f2f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.98ms - 34.95ms	-	slower ❌ 0% - 4% 0.14ms - 1.41ms	unsure 🔍 -1% - +3% -0.40ms - +0.90ms
tip-of-tree tip-of-tree	33.28ms - 34.10ms	faster ✔ 0% - 4% 0.14ms - 1.41ms	-	unsure 🔍 -3% - +0% -1.13ms - +0.07ms
previous-release previous-release	33.78ms - 34.66ms	unsure 🔍 -3% - +1% -0.90ms - +0.40ms	unsure 🔍 -0% - +3% -0.07ms - +1.13ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	72.87ms - 74.86ms	-	unsure 🔍 -0% - +4% -0.02ms - +2.56ms	slower ❌ 0% - 3% 0.18ms - 2.48ms
tip-of-tree tip-of-tree	71.77ms - 73.41ms	unsure 🔍 -3% - +0% -2.56ms - +0.02ms	-	unsure 🔍 -1% - +1% -0.94ms - +1.06ms
previous-release previous-release	71.96ms - 73.11ms	faster ✔ 0% - 3% 0.18ms - 2.48ms	unsure 🔍 -1% - +1% -1.06ms - +0.94ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6065

• Commit: 7918f2f

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.94ms - 34.78ms	-	unsure 🔍 -3% - +1% -0.89ms - +0.32ms	unsure 🔍 -2% - +2% -0.62ms - +0.58ms
tip-of-tree tip-of-tree	34.20ms - 35.08ms	unsure 🔍 -1% - +3% -0.32ms - +0.89ms	-	unsure 🔍 -1% - +3% -0.35ms - +0.88ms
previous-release previous-release	33.95ms - 34.80ms	unsure 🔍 -2% - +2% -0.58ms - +0.62ms	unsure 🔍 -3% - +1% -0.88ms - +0.35ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	543.85ms - 549.19ms	-	unsure 🔍 -0% - +1% -1.78ms - +4.73ms	unsure 🔍 -1% - +0% -5.06ms - +2.27ms
tip-of-tree tip-of-tree	543.18ms - 546.91ms	unsure 🔍 -1% - +0% -4.73ms - +1.78ms	-	unsure 🔍 -1% - +0% -6.00ms - +0.26ms
previous-release previous-release	545.40ms - 550.43ms	unsure 🔍 -0% - +1% -2.27ms - +5.06ms	unsure 🔍 -0% - +1% -0.26ms - +6.00ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	553.19ms - 558.52ms	-	unsure 🔍 -1% - +1% -4.03ms - +3.55ms	unsure 🔍 -1% - +0% -5.02ms - +2.65ms
tip-of-tree tip-of-tree	553.39ms - 558.79ms	unsure 🔍 -1% - +1% -3.55ms - +4.03ms	-	unsure 🔍 -1% - +1% -4.80ms - +2.91ms
previous-release previous-release	554.28ms - 559.79ms	unsure 🔍 -0% - +1% -2.65ms - +5.02ms	unsure 🔍 -1% - +1% -2.91ms - +4.80ms	-

_{tachometer-reporter-action v2 for Benchmarks}

[github-actions]

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

[augustjk]

It might not be the best fix but it's a very minimal patch that fixes a bad behavior.
I'm in favor of doing something quick to fix this, and deliberating on a better architecture for using private ssr support.

[AndrewJakubowicz]

For additional context, I'm not declaring bankruptcy on the better solution. But this solution is super small, fixes the leak, and doesn't block the better solution.
In the short term, this fix lets users of SSR test to see if their detected leaks have stopped.

[AndrewJakubowicz]

Went with the better approach in the end: #4515