[lit-html] Fix `ref` bug when auto-bound class method used as as callback by kevinpschaaf · Pull Request #2691 · lit/lit

kevinpschaaf · 2022-03-31T21:39:09Z

The WeakMap used to store the last element called to a given callback is keyed on the callback function. Since unbound LitElement class methods passed as callbacks will all have the same references between class instances, this will result in incorrect tracking of the last element per unique callback, given that callbacks called on different context's should be considered unique.

The change here uses nested WeakMaps to effectively double-key the lookup on both function and context.

Fixes #2677.

changeset-bot · 2022-03-31T21:39:11Z

🦋 Changeset detected

Latest commit: e915bde

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 · 2022-03-31T21:39:32Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -5% - +9% (-1.72ms - +3.28ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -3% - +2% (-3.10ms - +2.45ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -3% - +4% (-1.31ms - +1.70ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +12% (-0.38ms - +1.59ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +3% (-2.11ms - +1.96ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +2% (-1.78ms - +1.36ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -2% - +2% (-24.33ms - +22.57ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -4% - +2% (-5.11ms - +2.13ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +6% (-11.33ms - +20.52ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +5% (-1.72ms - +7.39ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +3% (-18.70ms - +28.15ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: unsure 🔍 -2% - +2% (-23.76ms - +21.29ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +3% (-14.42ms - +32.54ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2654
Commit: 7aa0530

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	104.27ms - 108.05ms	-	unsure 🔍 -3% - +2% -3.10ms - +2.45ms	faster ✔ 19% - 24% 24.72ms - 33.20ms
tip-of-tree tip-of-tree	104.45ms - 108.51ms	unsure 🔍 -2% - +3% -2.45ms - +3.10ms	-	faster ✔ 19% - 24% 24.33ms - 32.94ms
previous-release previous-release	131.32ms - 138.92ms	slower ❌ 23% - 32% 24.72ms - 33.20ms	slower ❌ 23% - 31% 24.33ms - 32.94ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	995.62ms - 1027.85ms	-	unsure 🔍 -2% - +2% -24.33ms - +22.57ms	faster ✔ 6% - 10% 61.75ms - 109.42ms
tip-of-tree tip-of-tree	995.59ms - 1029.65ms	unsure 🔍 -2% - +2% -22.57ms - +24.33ms	-	faster ✔ 6% - 10% 60.24ms - 109.17ms
previous-release previous-release	1079.76ms - 1114.88ms	slower ❌ 6% - 11% 61.75ms - 109.42ms	slower ❌ 6% - 11% 60.24ms - 109.17ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1085.25ms - 1114.24ms	-	unsure 🔍 -2% - +2% -23.76ms - +21.29ms	faster ✔ 3% - 7% 39.87ms - 85.80ms
tip-of-tree tip-of-tree	1083.74ms - 1118.22ms	unsure 🔍 -2% - +2% -21.29ms - +23.76ms	-	faster ✔ 3% - 7% 36.82ms - 86.39ms
previous-release previous-release	1144.77ms - 1180.39ms	slower ❌ 4% - 8% 39.87ms - 85.80ms	slower ❌ 3% - 8% 36.82ms - 86.39ms	-

lit-html-kitchen-sink

Browser: chrome-headless 98.0.4758.102
Sample size: 30
Built by: Benchmarks #2654
Commit: 7aa0530

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	42.33ms - 44.77ms	-	unsure 🔍 -3% - +4% -1.31ms - +1.70ms	faster ✔ 10% - 17% 4.70ms - 8.49ms
tip-of-tree tip-of-tree	42.47ms - 44.24ms	unsure 🔍 -4% - +3% -1.70ms - +1.31ms	-	faster ✔ 10% - 17% 5.09ms - 8.49ms
previous-release previous-release	48.70ms - 51.60ms	slower ❌ 11% - 20% 4.70ms - 8.49ms	slower ❌ 12% - 20% 5.09ms - 8.49ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	111.04ms - 115.97ms	-	unsure 🔍 -4% - +2% -5.11ms - +2.13ms	faster ✔ 1% - 8% 1.41ms - 9.40ms
tip-of-tree tip-of-tree	112.34ms - 117.64ms	unsure 🔍 -2% - +5% -2.13ms - +5.11ms	-	unsure 🔍 -7% - +0% -8.03ms - +0.19ms
previous-release previous-release	115.77ms - 122.06ms	slower ❌ 1% - 8% 1.41ms - 9.40ms	unsure 🔍 -0% - +7% -0.19ms - +8.03ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.77ms - 38.59ms	-	unsure 🔍 -5% - +9% -1.72ms - +3.28ms	unsure 🔍 -12% - +1% -4.81ms - +0.31ms
tip-of-tree tip-of-tree	34.74ms - 36.06ms	unsure 🔍 -9% - +5% -3.28ms - +1.72ms	-	faster ✔ 5% - 11% 1.94ms - 4.13ms
previous-release previous-release	37.56ms - 39.30ms	unsure 🔍 -1% - +14% -0.31ms - +4.81ms	slower ❌ 5% - 12% 1.94ms - 4.13ms	-

lit-html-repeat

Browser: chrome-headless 98.0.4758.102
Sample size: 30
Built by: Benchmarks #2654
Commit: 7aa0530

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	13.73ms - 15.30ms	-	unsure 🔍 -3% - +12% -0.38ms - +1.59ms	faster ✔ 2% - 13% 0.24ms - 2.00ms
tip-of-tree tip-of-tree	13.32ms - 14.50ms	unsure 🔍 -11% - +2% -1.59ms - +0.38ms	-	faster ✔ 7% - 15% 1.01ms - 2.44ms
previous-release previous-release	15.24ms - 16.04ms	slower ❌ 1% - 14% 0.24ms - 2.00ms	slower ❌ 7% - 18% 1.01ms - 2.44ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	358.98ms - 385.28ms	-	unsure 🔍 -3% - +6% -11.33ms - +20.52ms	faster ✔ 29% - 34% 154.23ms - 187.27ms
tip-of-tree tip-of-tree	358.55ms - 376.51ms	unsure 🔍 -5% - +3% -20.52ms - +11.33ms	-	faster ✔ 30% - 34% 161.91ms - 188.79ms
previous-release previous-release	532.88ms - 552.88ms	slower ❌ 40% - 52% 154.23ms - 187.27ms	slower ❌ 43% - 52% 161.91ms - 188.79ms	-

lit-html-template-heavy

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2654
Commit: 7aa0530

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	68.78ms - 71.33ms	-	unsure 🔍 -3% - +3% -2.11ms - +1.96ms	faster ✔ 14% - 18% 11.65ms - 15.20ms
tip-of-tree tip-of-tree	68.55ms - 71.72ms	unsure 🔍 -3% - +3% -1.96ms - +2.11ms	-	faster ✔ 14% - 18% 11.34ms - 15.36ms
previous-release previous-release	82.25ms - 84.72ms	slower ❌ 16% - 22% 11.65ms - 15.20ms	slower ❌ 16% - 22% 11.34ms - 15.36ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	151.56ms - 158.16ms	-	unsure 🔍 -1% - +5% -1.72ms - +7.39ms	faster ✔ 9% - 14% 16.16ms - 25.68ms
tip-of-tree tip-of-tree	148.88ms - 155.16ms	unsure 🔍 -5% - +1% -7.39ms - +1.72ms	-	faster ✔ 11% - 16% 19.10ms - 28.40ms
previous-release previous-release	172.35ms - 179.20ms	slower ❌ 10% - 17% 16.16ms - 25.68ms	slower ❌ 12% - 19% 19.10ms - 28.40ms	-

reactive-element-list

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2654
Commit: 7aa0530

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	71.53ms - 73.85ms	-	unsure 🔍 -2% - +2% -1.78ms - +1.36ms	unsure 🔍 -3% - +2% -2.05ms - +1.11ms
tip-of-tree tip-of-tree	71.84ms - 73.97ms	unsure 🔍 -2% - +2% -1.36ms - +1.78ms	-	unsure 🔍 -2% - +2% -1.77ms - +1.25ms
previous-release previous-release	72.09ms - 74.24ms	unsure 🔍 -2% - +3% -1.11ms - +2.05ms	unsure 🔍 -2% - +2% -1.25ms - +1.77ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1039.42ms - 1071.51ms	-	unsure 🔍 -2% - +3% -18.70ms - +28.15ms	unsure 🔍 -2% - +2% -18.49ms - +26.11ms
tip-of-tree tip-of-tree	1033.67ms - 1067.81ms	unsure 🔍 -3% - +2% -28.15ms - +18.70ms	-	unsure 🔍 -2% - +2% -23.97ms - +22.13ms
previous-release previous-release	1036.17ms - 1067.15ms	unsure 🔍 -2% - +2% -26.11ms - +18.49ms	unsure 🔍 -2% - +2% -22.13ms - +23.97ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1145.64ms - 1181.92ms	-	unsure 🔍 -1% - +3% -14.42ms - +32.54ms	unsure 🔍 -1% - +3% -14.50ms - +31.96ms
tip-of-tree tip-of-tree	1139.81ms - 1169.63ms	unsure 🔍 -3% - +1% -32.54ms - +14.42ms	-	unsure 🔍 -2% - +2% -21.14ms - +20.48ms
previous-release previous-release	1140.53ms - 1169.57ms	unsure 🔍 -3% - +1% -31.96ms - +14.50ms	unsure 🔍 -2% - +2% -20.48ms - +21.14ms	-

_{tachometer-reporter-action v2 for Benchmarks}

sorvell · 2022-03-31T22:25:40Z

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

+// has already been rendered to a new spot. It is double-keyed on both the
+// callback and the context (options.host), since we auto-bind class methods to
+// options.host.
+const lastElementForCallback: WeakMap<


It seems like the WeakMap setup would make more sense if it was keyed off the host first.

.changeset/short-cooks-deliver.md

packages/lit-html/src/test/directives/ref_test.ts

Co-authored-by: Andrew Jakubowicz <spyr1014@gmail.com>

AndrewJakubowicz

Reviewed - although primarily syntax and logic surrounding the issue, since lack expertise in this domain.
This is awesome! TIL! Test is fantastic, and only comments are nits and clarifying my own understanding.

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

AndrewJakubowicz

Brilliant!

kevinpschaaf added 2 commits March 31, 2022 14:29

[lit-html] Key "last element" map on context in addition to callback

0772428

Add changeset

2608bf6

kevinpschaaf requested a review from justinfagnani as a code owner March 31, 2022 21:39

kevinpschaaf requested a review from sorvell March 31, 2022 21:39

sorvell requested changes Mar 31, 2022

View reviewed changes

Flip order of context and callback lookups

0712cec

AndrewJakubowicz reviewed Mar 31, 2022

View reviewed changes

.changeset/short-cooks-deliver.md Show resolved Hide resolved

Add lit to changeset

e4dece0

AndrewJakubowicz reviewed Mar 31, 2022

View reviewed changes

.changeset/short-cooks-deliver.md Outdated Show resolved Hide resolved

packages/lit-html/src/test/directives/ref_test.ts Show resolved Hide resolved

Update .changeset/short-cooks-deliver.md

8dcaa01

Co-authored-by: Andrew Jakubowicz <spyr1014@gmail.com>

AndrewJakubowicz approved these changes Apr 1, 2022

View reviewed changes

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

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

sorvell approved these changes Apr 1, 2022

View reviewed changes

Updates based on feedback

e915bde

AndrewJakubowicz approved these changes Apr 1, 2022

View reviewed changes

kevinpschaaf added the cla: yes label Apr 1, 2022

kevinpschaaf merged commit 171143b into main Apr 1, 2022

kevinpschaaf deleted the ref-bound-callback-fix branch April 1, 2022 17:11

lit-robot mentioned this pull request Apr 4, 2022

Version Packages #2700

Merged

Conversation

Uh oh!

Uh oh!

🦋 Changeset detected

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants