Skip to content

Conversation

@kevinpschaaf
Copy link
Member

@kevinpschaaf kevinpschaaf commented Aug 4, 2021

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
Copy link

changeset-bot bot commented Aug 4, 2021

🦋 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

@google-cla google-cla bot added the cla: yes label Aug 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2021

📊 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

render

VersionAvg timevs 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.56msunsure 🔍
-2% - +1%
-2.85ms - +0.94ms
-faster ✔
22% - 24%
34.20ms - 38.22ms
previous-release
previous-release
153.02ms - 156.51msslower ❌
27% - 32%
32.89ms - 37.62ms
slower ❌
29% - 32%
34.20ms - 38.22ms
-

update

VersionAvg timevs 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.86msunsure 🔍
-1% - +2%
-13.16ms - +16.65ms
-faster ✔
7% - 10%
77.41ms - 109.07ms
previous-release
previous-release
1091.87ms - 1116.68msslower ❌
8% - 11%
78.27ms - 111.70ms
slower ❌
8% - 11%
77.41ms - 109.07ms
-

update-reflect

VersionAvg timevs 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.08msunsure 🔍
-0% - +3%
-2.95ms - +29.57ms
-faster ✔
5% - 8%
50.19ms - 83.69ms
previous-release
previous-release
1096.34ms - 1120.52msslower ❌
6% - 9%
63.63ms - 96.87ms
slower ❌
5% - 8%
50.19ms - 83.69ms
-
lit-html-kitchen-sink

render

VersionAvg timevs 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.53msunsure 🔍
-1% - +3%
-0.55ms - +1.31ms
-faster ✔
17% - 21%
9.76ms - 12.86ms
previous-release
previous-release
57.73ms - 60.43msslower ❌
21% - 28%
10.23ms - 13.14ms
slower ❌
20% - 27%
9.76ms - 12.86ms
-

update

VersionAvg timevs 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.73msunsure 🔍
-3% - +3%
-3.90ms - +3.71ms
-faster ✔
3% - 8%
4.09ms - 11.21ms
previous-release
previous-release
129.36ms - 134.75msslower ❌
3% - 9%
3.52ms - 11.60ms
slower ❌
3% - 9%
4.09ms - 11.21ms
-

nop-update

VersionAvg timevs 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.67msunsure 🔍
-2% - +4%
-0.65ms - +1.87ms
-unsure 🔍
-1% - +5%
-0.49ms - +2.11ms
previous-release
previous-release
42.04ms - 43.58msunsure 🔍
-3% - +2%
-1.24ms - +0.83ms
unsure 🔍
-5% - +1%
-2.11ms - +0.49ms
-
lit-html-repeat

render

VersionAvg timevs 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.05msunsure 🔍
-7% - +6%
-1.03ms - +0.96ms
-faster ✔
5% - 34%
0.26ms - 7.18ms
previous-release
previous-release
15.88ms - 22.74msslower ❌
1% - 47%
0.15ms - 7.22ms
slower ❌
2% - 46%
0.26ms - 7.18ms
-

update

VersionAvg timevs 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.12msunsure 🔍
-6% - +6%
-31.28ms - +31.86ms
-faster ✔
25% - 34%
182.55ms - 257.86ms
previous-release
previous-release
716.73ms - 777.66msslower ❌
33% - 50%
182.62ms - 258.38ms
slower ❌
33% - 50%
182.55ms - 257.86ms
-
lit-html-template-heavy

render

VersionAvg timevs 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.38msunsure 🔍
-1% - +4%
-0.54ms - +2.95ms
-faster ✔
14% - 17%
13.07ms - 16.09ms
previous-release
previous-release
91.84ms - 93.79msslower ❌
18% - 23%
14.15ms - 17.42ms
slower ❌
16% - 21%
13.07ms - 16.09ms
-

update

VersionAvg timevs 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.74msunsure 🔍
-2% - +3%
-3.33ms - +5.38ms
-faster ✔
9% - 14%
16.79ms - 25.23ms
previous-release
previous-release
181.82ms - 187.16msslower ❌
11% - 16%
18.10ms - 25.96ms
slower ❌
10% - 16%
16.79ms - 25.23ms
-
reactive-element-list

render

VersionAvg timevs 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.90msunsure 🔍
-3% - +0%
-2.59ms - +0.35ms
-unsure 🔍
-2% - +2%
-1.35ms - +1.61ms
previous-release
previous-release
77.68ms - 79.64msunsure 🔍
-3% - +0%
-2.62ms - +0.13ms
unsure 🔍
-2% - +2%
-1.61ms - +1.35ms
-

update

VersionAvg timevs 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.35msunsure 🔍
-2% - +1%
-22.46ms - +6.51ms
-unsure 🔍
-2% - +1%
-17.46ms - +11.04ms
previous-release
previous-release
973.40ms - 993.14msunsure 🔍
-2% - +1%
-18.96ms - +9.43ms
unsure 🔍
-1% - +2%
-11.04ms - +17.46ms
-

update-reflect

VersionAvg timevs 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.25msunsure 🔍
-2% - +1%
-19.19ms - +11.88ms
-unsure 🔍
-2% - +1%
-22.22ms - +9.73ms
previous-release
previous-release
1053.90ms - 1076.59msunsure 🔍
-1% - +2%
-13.02ms - +18.20ms
unsure 🔍
-1% - +2%
-9.73ms - +22.22ms
-

tachometer-reporter-action v2 for Benchmarks

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@kevinpschaaf kevinpschaaf marked this pull request as ready for review August 10, 2021 01:34
/** @internal */
_$parent: Disconnectable | undefined;
/** @internal */
__isConnected = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be initialized via options?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this distinct from this.options' isConnected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test to verify that updates happen while disconnected?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could call this just RootPart

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


constructor(_partInfo: PartInfo) {}

get _$isConnected() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment

clearPart(containerPart);
insertPart(containerPart, undefined, cachedPart);
setCommittedValue(containerPart, [cachedPart]);
cachedPart.setConnected(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is removed because insertPart handles it now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right

if (cachedContainerPart === undefined) {
const fragment = document.createDocumentFragment();
cachedContainerPart = render(nothing, fragment);
cachedContainerPart.setConnected(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is set earlier here because insertPart synchronizes the connection state?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, previously we changed the connection state on the ChildPart that we inserted and moved between the cache and the container parts as we moved them. Now, cachedContainerPart is a RootChildPart, so we set it to "disconnected" once, and each time we move a part in and out, its connection state will be synced to the root part it is in.

this._$parent = parent;
}

get _$isConnected() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment describing this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented once in the interface and added comments for each one pointing to it.

As opposed to other Disconnectables, AsyncDirectives always get notified when the RootChildPart connection changes, so the public `isConnected` available to end users can just be the local value, not a getter.
newValues[this.__attributeIndex!] = value;
(this.__part as AttributePart)._$setValue(newValues, this, 0);
}
if (isSingleExpression(this.__part as unknown as PartInfo)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs above this still say the value is queued. Need to update that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, done.


export const memorySuite = canRunMemoryTests ? suite : suite.skip;

export const forceGC = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wan to add a comment about why this is being called 10x.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file actually goes with the next PR. Will test there and remove if unneeded.

sorvell
sorvell approved these changes Aug 13, 2021
}
}
/** @internal */
setConnected(isConnected: boolean) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still add some docs that say this should only be called on root parts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* of `AsyncDirective`s created throughout the tree below it.
*/
export interface RootChildPart extends ChildPart {
setConnected(isConnected: boolean): void;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add jsdocs here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@justinfagnani
Copy link
Collaborator

LGTM, though I still prefer the name RootPart. RootChildPart just sounds a bit awkward and contradictory to me.

@kevinpschaaf
Copy link
Member Author

@justinfagnani Ok I think I changed my mind. I liked RootChildPart from the point of view of it being important for users to understand that it had all the API of a ChildPart plus the RootPart API (setConnected). But on further reflection, there's actually very-little-to-no useful ChildPart API that a user would need to know about/use on the value returned from render. So I think I'm good with that just being RootPart.

@kevinpschaaf kevinpschaaf merged commit 5768cc6 into main Aug 17, 2021
@kevinpschaaf kevinpschaaf deleted the revert-disconnected-updates branch August 17, 2021 05:25
kevinpschaaf added a commit to lit/lit.dev that referenced this pull request Aug 17, 2021
kevinpschaaf added a commit to lit/lit.dev that referenced this pull request Aug 20, 2021
* Sync disconnection-related docs with changes in lit/#2034
See lit/lit#2034

* Update packages/lit-dev-content/site/docs/templates/custom-directives.md

Co-authored-by: Justin Fagnani <[email protected]>

* Updates based on feedback

* Typos

Co-authored-by: Justin Fagnani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants