[lit-html, reactive-element] Revert change to pause update cycle while disconnected #2034

kevinpschaaf · 2021-08-04T00:12:33Z

Reverts the change in Lit 2 to pause ReactiveElement's update cycle while the element is disconnected.

This change was originally added to Lit 2 to ensure that once AsyncDirectives are disconnected, they do not receive further updates until they are reconnected.

However, the change in Lit 2 to pause updates while the element was disconnected was found during internal testing to be subtly breaking in potentially dangerous ways, which could lead to memory leaks if the update cycle was relied on to clean up external event handlers/subscriptions.

Instead, the onus will shift to AsyncDirective authors to check the this.isConnected flag during update and avoid doing any potentially leaky work while disconnected, since they may never be reconnected in that state.

Note that calling setValue from an asyncDirective while disconnected will now synchronously commit the change (since it seems arbitrary now to not do that; that was only in to maintain the same guarantee we're removing above (that disconnected directives don't receive updates).

A follow-on PR (#2044) re-works the disconnection handling for first-party AsyncDirectives (until, asyncAppend, asyncReplace) to allow them to be GC'ed before their awaited promises resolve.

lit.dev documentation updates covering this change are in lit.dev#436.

changeset-bot · 2021-08-04T00:12:34Z

🦋 Changeset detected

Latest commit: a54c3dc

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

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 · 2021-08-04T00:12:56Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -4% - +1% (-1.87ms - +0.65ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -1% - +2% (-0.94ms - +2.85ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -3% - +1% (-1.31ms - +0.55ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +7% (-0.96ms - +1.03ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -4% - +1% (-2.95ms - +0.54ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +3% (-0.35ms - +2.59ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -2% - +1% (-16.65ms - +13.16ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -3% - +3% (-3.71ms - +3.90ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +6% (-31.86ms - +31.28ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +2% (-5.38ms - +3.33ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-6.51ms - +22.46ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -3% - +0% (-29.57ms - +2.95ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-11.88ms - +19.19ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1661
Commit: a6c49a1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	117.90ms - 121.11ms	-	unsure 🔍 -1% - +2% -0.94ms - +2.85ms	faster ✔ 21% - 24% 32.89ms - 37.62ms
tip-of-tree tip-of-tree	117.55ms - 119.56ms	unsure 🔍 -2% - +1% -2.85ms - +0.94ms	-	faster ✔ 22% - 24% 34.20ms - 38.22ms
previous-release previous-release	153.02ms - 156.51ms	slower ❌ 27% - 32% 32.89ms - 37.62ms	slower ❌ 29% - 32% 34.20ms - 38.22ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	998.09ms - 1020.49ms	-	unsure 🔍 -2% - +1% -16.65ms - +13.16ms	faster ✔ 7% - 10% 78.27ms - 111.70ms
tip-of-tree tip-of-tree	1001.20ms - 1020.86ms	unsure 🔍 -1% - +2% -13.16ms - +16.65ms	-	faster ✔ 7% - 10% 77.41ms - 109.07ms
previous-release previous-release	1091.87ms - 1116.68ms	slower ❌ 8% - 11% 78.27ms - 111.70ms	slower ❌ 8% - 11% 77.41ms - 109.07ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1016.77ms - 1039.58ms	-	unsure 🔍 -3% - +0% -29.57ms - +2.95ms	faster ✔ 6% - 9% 63.63ms - 96.87ms
tip-of-tree tip-of-tree	1029.90ms - 1053.08ms	unsure 🔍 -0% - +3% -2.95ms - +29.57ms	-	faster ✔ 5% - 8% 50.19ms - 83.69ms
previous-release previous-release	1096.34ms - 1120.52ms	slower ❌ 6% - 9% 63.63ms - 96.87ms	slower ❌ 5% - 8% 50.19ms - 83.69ms	-

lit-html-kitchen-sink

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1661
Commit: a6c49a1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	46.86ms - 47.93ms	-	unsure 🔍 -3% - +1% -1.31ms - +0.55ms	faster ✔ 18% - 22% 10.23ms - 13.14ms
tip-of-tree tip-of-tree	47.01ms - 48.53ms	unsure 🔍 -1% - +3% -0.55ms - +1.31ms	-	faster ✔ 17% - 21% 9.76ms - 12.86ms
previous-release previous-release	57.73ms - 60.43ms	slower ❌ 21% - 28% 10.23ms - 13.14ms	slower ❌ 20% - 27% 9.76ms - 12.86ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	121.49ms - 127.50ms	-	unsure 🔍 -3% - +3% -3.71ms - +3.90ms	faster ✔ 3% - 9% 3.52ms - 11.60ms
tip-of-tree tip-of-tree	122.08ms - 126.73ms	unsure 🔍 -3% - +3% -3.90ms - +3.71ms	-	faster ✔ 3% - 8% 4.09ms - 11.21ms
previous-release previous-release	129.36ms - 134.75ms	slower ❌ 3% - 9% 3.52ms - 11.60ms	slower ❌ 3% - 9% 4.09ms - 11.21ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	42.32ms - 43.70ms	-	unsure 🔍 -4% - +1% -1.87ms - +0.65ms	unsure 🔍 -2% - +3% -0.83ms - +1.24ms
tip-of-tree tip-of-tree	42.57ms - 44.67ms	unsure 🔍 -2% - +4% -0.65ms - +1.87ms	-	unsure 🔍 -1% - +5% -0.49ms - +2.11ms
previous-release previous-release	42.04ms - 43.58ms	unsure 🔍 -3% - +2% -1.24ms - +0.83ms	unsure 🔍 -5% - +1% -2.11ms - +0.49ms	-

lit-html-repeat

Browser: chrome-headless 92.0.4515.131
Sample size: 30
Built by: Benchmarks #1661
Commit: a6c49a1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	14.74ms - 16.51ms	-	unsure 🔍 -6% - +7% -0.96ms - +1.03ms	faster ✔ 4% - 34% 0.15ms - 7.22ms
tip-of-tree tip-of-tree	15.13ms - 16.05ms	unsure 🔍 -7% - +6% -1.03ms - +0.96ms	-	faster ✔ 5% - 34% 0.26ms - 7.18ms
previous-release previous-release	15.88ms - 22.74ms	slower ❌ 1% - 47% 0.15ms - 7.22ms	slower ❌ 2% - 46% 0.26ms - 7.18ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	504.19ms - 549.21ms	-	unsure 🔍 -6% - +6% -31.86ms - +31.28ms	faster ✔ 25% - 34% 182.62ms - 258.38ms
tip-of-tree tip-of-tree	504.86ms - 549.12ms	unsure 🔍 -6% - +6% -31.28ms - +31.86ms	-	faster ✔ 25% - 34% 182.55ms - 257.86ms
previous-release previous-release	716.73ms - 777.66ms	slower ❌ 33% - 50% 182.62ms - 258.38ms	slower ❌ 33% - 50% 182.55ms - 257.86ms	-

lit-html-template-heavy

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1661
Commit: a6c49a1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	75.72ms - 78.34ms	-	unsure 🔍 -4% - +1% -2.95ms - +0.54ms	faster ✔ 15% - 19% 14.15ms - 17.42ms
tip-of-tree tip-of-tree	77.08ms - 79.38ms	unsure 🔍 -1% - +4% -0.54ms - +2.95ms	-	faster ✔ 14% - 17% 13.07ms - 16.09ms
previous-release previous-release	91.84ms - 93.79ms	slower ❌ 18% - 23% 14.15ms - 17.42ms	slower ❌ 16% - 21% 13.07ms - 16.09ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	159.57ms - 165.33ms	-	unsure 🔍 -3% - +2% -5.38ms - +3.33ms	faster ✔ 10% - 14% 18.10ms - 25.96ms
tip-of-tree tip-of-tree	160.21ms - 166.74ms	unsure 🔍 -2% - +3% -3.33ms - +5.38ms	-	faster ✔ 9% - 14% 16.79ms - 25.23ms
previous-release previous-release	181.82ms - 187.16ms	slower ❌ 11% - 16% 18.10ms - 25.96ms	slower ❌ 10% - 16% 16.79ms - 25.23ms	-

reactive-element-list

Browser: chrome-headless 92.0.4515.131
Sample size: 50
Built by: Benchmarks #1661
Commit: a6c49a1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	78.94ms - 80.88ms	-	unsure 🔍 -0% - +3% -0.35ms - +2.59ms	unsure 🔍 -0% - +3% -0.13ms - +2.62ms
tip-of-tree tip-of-tree	77.68ms - 79.90ms	unsure 🔍 -3% - +0% -2.59ms - +0.35ms	-	unsure 🔍 -2% - +2% -1.35ms - +1.61ms
previous-release previous-release	77.68ms - 79.64ms	unsure 🔍 -3% - +0% -2.62ms - +0.13ms	unsure 🔍 -2% - +2% -1.61ms - +1.35ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	977.84ms - 998.24ms	-	unsure 🔍 -1% - +2% -6.51ms - +22.46ms	unsure 🔍 -1% - +2% -9.43ms - +18.96ms
tip-of-tree tip-of-tree	969.77ms - 990.35ms	unsure 🔍 -2% - +1% -22.46ms - +6.51ms	-	unsure 🔍 -2% - +1% -17.46ms - +11.04ms
previous-release previous-release	973.40ms - 993.14ms	unsure 🔍 -2% - +1% -18.96ms - +9.43ms	unsure 🔍 -1% - +2% -11.04ms - +17.46ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1051.93ms - 1073.37ms	-	unsure 🔍 -1% - +2% -11.88ms - +19.19ms	unsure 🔍 -2% - +1% -18.20ms - +13.02ms
tip-of-tree tip-of-tree	1047.75ms - 1070.25ms	unsure 🔍 -2% - +1% -19.19ms - +11.88ms	-	unsure 🔍 -2% - +1% -22.22ms - +9.73ms
previous-release previous-release	1053.90ms - 1076.59ms	unsure 🔍 -1% - +2% -13.02ms - +18.20ms	unsure 🔍 -1% - +2% -9.73ms - +22.22ms	-

_{tachometer-reporter-action v2 for Benchmarks}

sorvell · 2021-08-04T21:52:32Z

packages/lit-html/src/async-directive.ts

    parent: Disconnectable,
    attributeIndex: number | undefined
  ) {
+    this.isConnected = (part.options as PrivateRenderOptions).isConnected;


Should we consider naming the private key in renderOptions more obtusely since users can pass custom values here and we don't want to conflict?

I think it's basically fine to conflict if we always overwrite. It's the same as ignore the user-provided value. If we only sometimes overwrote it'd be an issue, IMO.

Since this is an implementation detail that we don't want anyone to even be reading from, I went ahead and renamed it to _$isConnected and made it a stableProperty.

Add isConnected handling to SSR Fix SSR test Re-work async directives for better gc behavior Allows the directives (and the DOM they are attached to) to be gc'ed when disconnected before awaited promises/iterables resolve Formatting Rename PrivateRenderOptions.isConnected to _ and make stable Ensure protected methods aren't renamed

packages/lit-html/src/lit-html.ts

packages/lit-html/src/experimental-hydrate.ts

packages/lit-html/src/lit-html.ts

sorvell · 2021-08-10T17:20:28Z

packages/lit-html/src/lit-html.ts

  /** @internal */
  _$parent: Disconnectable | undefined;
+  /** @internal */
+  __isConnected = true;


Should this be initialized via options?

Why is this distinct from this.options' isConnected?

The connection state of the top-level part (the part created/returned by render that the user can call part.setConnection(true/false) on) is the source-of-truth for everything below it. The options._$isConnected flag is not the source of truth, but rather a communication channel between the top-level part and everything below it since they all get references to this one options bag passed down.

Should this be initialized via options?

In theory, the __isConnected flag on any parts below the top-level part returned is never used (and the top-level part should never get an options bag with _$isConnected in it), so it doesn't really matter, but we could keep it in sync just for consistency.

I think there's a flaw though... It's possible to get a reference to a child part that shares options with the top-level part, and then call part.setConnected(false) and change the connection state. From there, if you did part.setValue(...), directives would still see the top-level connection state, not the local one.

Per design conversation, re-worked to use get _$isConnected() { return this._$parent._$isConnected } on all Disconnectables up to the RootChildPart, which holds the state for the entire tree.

And we're TODO'ing setting the initial connected state in render because in LitElement it's almost always going to be first rendered when connected, yeah?

Yeah, added TODO's, a skipped failing test for this (https://github.com/lit/lit/pull/2034/files#diff-fc790d086c06e1adeb4c976a60945f0b9b50aa86350a9c9c6b61618c48bac95dR522), and issue #2051

packages/lit-html/src/lit-html.ts

packages/lit-html/src/test/lit-html_test.ts

sorvell · 2021-08-10T17:29:06Z

packages/reactive-element/src/test/reactive-element_test.ts

      assert.equal(a.updateCount, 4);
    });
  });



Add a test to verify that updates happen while disconnected?

Co-authored-by: Steve Orvell <[email protected]>

justinfagnani · 2021-08-11T18:09:15Z

packages/lit-html/src/lit-html.ts

+ * of `AsyncDirective`s created throughout the tree below it.
+ */
+export type {RootChildPart};
+class RootChildPart extends ChildPart {


I think we could call this just RootPart

It didn't change the size, so I liked the clarity of RootChildPart.

packages/lit-html/src/lit-html.ts

packages/lit-html/src/directive-helpers.ts

packages/reactive-element/src/reactive-element.ts

packages/lit-html/src/test/test-utils/memory.ts

packages/lit-html/src/lit-html.ts

sorvell · 2021-08-13T13:11:48Z

packages/lit-html/src/directive-helpers.ts

+      // read it once we know the subtree has directives that need
+      // to be notified
+      let newConnectionState;
+      if (


Why was this not needed before? Was it just an omission? If so did we add test coverage?

Let's also add a comment here explaining that when a part is moved, its connection state needs to be updated to match the part it's moved into.

Not an omission per se; prior to this the user could change any part's connection state, so it was up to the user moving a part to keep it in sync.

With this change, only the root part holds connection state, and everything under it is kept in sync / notified automatically. Added a test in directive-helpers_test.ts.

sorvell · 2021-08-13T13:14:51Z

packages/lit-html/src/directive.ts


  constructor(_partInfo: PartInfo) {}

+  get _$isConnected() {


Add a comment here about how this works and why it's here.

Added a comment

google-cla bot added the cla: yes label Aug 4, 2021

sorvell requested changes Aug 4, 2021

View reviewed changes

kevinpschaaf added 2 commits August 9, 2021 18:10

Move directive changes to follow-on PR

422478e

kevinpschaaf force-pushed the revert-disconnected-updates branch from 5036725 to 422478e Compare August 10, 2021 01:24

kevinpschaaf mentioned this pull request Aug 10, 2021

[lit-html] Re-work async directives for better gc behavior #2044

Merged

Add changeset

9682937

kevinpschaaf marked this pull request as ready for review August 10, 2021 01:34

Make copy of user options since we're modifying it

2926046

justinfagnani approved these changes Aug 10, 2021

View reviewed changes

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

packages/lit-html/src/experimental-hydrate.ts Outdated Show resolved Hide resolved

sorvell requested changes Aug 10, 2021

View reviewed changes

kevinpschaaf and others added 8 commits August 10, 2021 10:45

Only copy options once when making part

114d1d4

Update packages/lit-html/src/test/lit-html_test.ts

fa3b1ea

Co-authored-by: Steve Orvell <[email protected]>

Get isConnected from parent via getter vs. options

d313272

Fixes for SSR

20fb5d7

Fix typo

6f53e2e

Add test for updates when disconnected

4ed25b3

Fix LitElement test

327f45b

Fix SSR build

4d650dc

justinfagnani requested changes Aug 11, 2021

View reviewed changes

kevinpschaaf added 3 commits August 11, 2021 12:41

Remove unused __enableConnection

453e6c2

Refactor logic

553a1eb

Remove unneeded assertion

0f385eb

sorvell requested changes Aug 13, 2021

View reviewed changes

[lit-html, reactive-element] Revert change to pause update cycle while disconnected #2034

[lit-html, reactive-element] Revert change to pause update cycle while disconnected #2034

Uh oh!

Conversation

kevinpschaaf commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Aug 4, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinpschaaf commented Aug 4, 2021 •

edited

Loading

changeset-bot bot commented Aug 4, 2021 •

edited

Loading

github-actions bot commented Aug 4, 2021 •

edited

Loading