[labs/virtualizer] Fix #3481 and #3518 by graynorton · Pull Request #3519 · lit/lit

graynorton · 2022-12-13T17:55:13Z

changeset-bot · 2022-12-13T17:55:16Z

🦋 Changeset detected

Latest commit: da8eb5f

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/virtualizer	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 · 2022-12-13T17:55:35Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -5% - +2% (-1.06ms - +0.51ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 85.02ms - 89.96ms
lit-html-kitchen-sink: unsure 🔍 -7% - +1% (-2.35ms - +0.41ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -5% - +5% (-0.58ms - +0.59ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-1.10ms - +1.10ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -4% - +1% (-2.21ms - +0.67ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 780.08ms - 792.97ms
lit-html-kitchen-sink: unsure 🔍 -5% - +2% (-4.14ms - +1.48ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +1% (-8.07ms - +2.44ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +2% (-1.40ms - +2.70ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-5.10ms - +12.58ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 779.98ms - 787.42ms
reactive-element-list: unsure 🔍 -1% - +1% (-5.08ms - +10.95ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 108.0.5359.94
Sample size: 50
Built by: Benchmarks #4187
Commit: f3b07c1

render

Version	Avg time	vs
	85.02ms - 89.96ms	-

update

Version	Avg time	vs
	780.08ms - 792.97ms	-

update-reflect

Version	Avg time	vs
	779.98ms - 787.42ms	-

lit-html-kitchen-sink

Browser: chrome-headless 108.0.5359.94
Sample size: 30
Built by: Benchmarks #4187
Commit: f3b07c1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.20ms - 32.46ms	-	unsure 🔍 -7% - +1% -2.35ms - +0.41ms	unsure 🔍 -5% - +2% -1.51ms - +0.76ms
tip-of-tree tip-of-tree	31.57ms - 34.03ms	unsure 🔍 -1% - +7% -0.41ms - +2.35ms	-	unsure 🔍 -3% - +7% -0.95ms - +2.14ms
previous-release previous-release	31.27ms - 33.15ms	unsure 🔍 -2% - +5% -0.76ms - +1.51ms	unsure 🔍 -6% - +3% -2.14ms - +0.95ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	83.19ms - 87.30ms	-	unsure 🔍 -5% - +2% -4.14ms - +1.48ms	unsure 🔍 -3% - +4% -2.67ms - +3.24ms
tip-of-tree tip-of-tree	84.65ms - 88.50ms	unsure 🔍 -2% - +5% -1.48ms - +4.14ms	-	unsure 🔍 -2% - +5% -1.26ms - +4.48ms
previous-release previous-release	82.84ms - 87.09ms	unsure 🔍 -4% - +3% -3.24ms - +2.67ms	unsure 🔍 -5% - +1% -4.48ms - +1.26ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	22.60ms - 23.56ms	-	unsure 🔍 -5% - +2% -1.06ms - +0.51ms	unsure 🔍 -3% - +3% -0.67ms - +0.65ms
tip-of-tree tip-of-tree	22.74ms - 23.98ms	unsure 🔍 -2% - +5% -0.51ms - +1.06ms	-	unsure 🔍 -2% - +4% -0.50ms - +1.04ms
previous-release previous-release	22.63ms - 23.55ms	unsure 🔍 -3% - +3% -0.65ms - +0.67ms	unsure 🔍 -4% - +2% -1.04ms - +0.50ms	-

lit-html-repeat

Browser: chrome-headless 108.0.5359.94
Sample size: 30
Built by: Benchmarks #4187
Commit: f3b07c1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.42ms - 12.24ms	-	unsure 🔍 -5% - +5% -0.58ms - +0.59ms	unsure 🔍 -8% - +1% -1.00ms - +0.12ms
tip-of-tree tip-of-tree	11.41ms - 12.24ms	unsure 🔍 -5% - +5% -0.59ms - +0.58ms	-	unsure 🔍 -8% - +1% -1.01ms - +0.12ms
previous-release previous-release	11.89ms - 12.65ms	unsure 🔍 -1% - +9% -0.12ms - +1.00ms	unsure 🔍 -1% - +9% -0.12ms - +1.01ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	297.67ms - 303.50ms	-	unsure 🔍 -3% - +1% -8.07ms - +2.44ms	faster ✔ 0% - 4% 1.24ms - 11.65ms
tip-of-tree tip-of-tree	299.03ms - 307.77ms	unsure 🔍 -1% - +3% -2.44ms - +8.07ms	-	unsure 🔍 -3% - +1% -9.77ms - +2.51ms
previous-release previous-release	302.72ms - 311.34ms	slower ❌ 0% - 4% 1.24ms - 11.65ms	unsure 🔍 -1% - +3% -2.51ms - +9.77ms	-

lit-html-template-heavy

Browser: chrome-headless 108.0.5359.94
Sample size: 50
Built by: Benchmarks #4187
Commit: f3b07c1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	54.69ms - 56.47ms	-	unsure 🔍 -2% - +2% -1.10ms - +1.10ms	unsure 🔍 -1% - +3% -0.77ms - +1.47ms
tip-of-tree tip-of-tree	54.94ms - 56.22ms	unsure 🔍 -2% - +2% -1.10ms - +1.10ms	-	unsure 🔍 -1% - +2% -0.58ms - +1.29ms
previous-release previous-release	54.55ms - 55.91ms	unsure 🔍 -3% - +1% -1.47ms - +0.77ms	unsure 🔍 -2% - +1% -1.29ms - +0.58ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	119.56ms - 122.42ms	-	unsure 🔍 -1% - +2% -1.40ms - +2.70ms	unsure 🔍 -1% - +2% -1.12ms - +2.73ms
tip-of-tree tip-of-tree	118.87ms - 121.82ms	unsure 🔍 -2% - +1% -2.70ms - +1.40ms	-	unsure 🔍 -2% - +2% -1.81ms - +2.11ms
previous-release previous-release	118.90ms - 121.48ms	unsure 🔍 -2% - +1% -2.73ms - +1.12ms	unsure 🔍 -2% - +1% -2.11ms - +1.81ms	-

reactive-element-list

Browser: chrome-headless 108.0.5359.94
Sample size: 50
Built by: Benchmarks #4187
Commit: f3b07c1

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.63ms - 55.58ms	-	unsure 🔍 -4% - +1% -2.21ms - +0.67ms	unsure 🔍 -3% - +2% -1.54ms - +1.18ms
tip-of-tree tip-of-tree	54.31ms - 56.44ms	unsure 🔍 -1% - +4% -0.67ms - +2.21ms	-	unsure 🔍 -2% - +4% -0.84ms - +2.02ms
previous-release previous-release	53.83ms - 55.74ms	unsure 🔍 -2% - +3% -1.18ms - +1.54ms	unsure 🔍 -4% - +2% -2.02ms - +0.84ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	795.10ms - 807.11ms	-	unsure 🔍 -1% - +2% -5.10ms - +12.58ms	unsure 🔍 -1% - +1% -5.50ms - +10.83ms
tip-of-tree tip-of-tree	790.88ms - 803.85ms	unsure 🔍 -2% - +1% -12.58ms - +5.10ms	-	unsure 🔍 -1% - +1% -9.60ms - +7.45ms
previous-release previous-release	792.90ms - 803.98ms	unsure 🔍 -1% - +1% -10.83ms - +5.50ms	unsure 🔍 -1% - +1% -7.45ms - +9.60ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	814.02ms - 826.58ms	-	unsure 🔍 -1% - +1% -5.08ms - +10.95ms	unsure 🔍 -0% - +2% -2.77ms - +12.89ms
tip-of-tree tip-of-tree	812.38ms - 822.34ms	unsure 🔍 -1% - +1% -10.95ms - +5.08ms	-	unsure 🔍 -1% - +1% -4.71ms - +8.95ms
previous-release previous-release	810.57ms - 819.90ms	unsure 🔍 -2% - +0% -12.89ms - +2.77ms	unsure 🔍 -1% - +1% -8.95ms - +4.71ms	-

_{tachometer-reporter-action v2 for Benchmarks}

usergenic

LGTM

usergenic · 2022-12-15T03:03:00Z

packages/labs/virtualizer/src/Virtualizer.ts

+    if (this._layoutCompletePromise && this._pendingLayoutComplete === null) {
+      // Wait one additional frame to be sure the layout is stable
+      this._pendingLayoutComplete = requestAnimationFrame(() =>
+        requestAnimationFrame(() => this._resolveLayoutCompletePromise())


Interesting! I like the locality/cleanliness of expressing the extra nesting RAF here. It appears to mean that resolving the layout complete promise is no longer deferrable past the second RAF, however, since this._resolveLayoutCompletePromise() is late bound and will be called "no matter what". I am trying to contrive a scenario where that would pose a risk...

Considering a case where _childrenSizeChanged() is called due to observation and that triggers a subsequent scheduleLayoutComplete but the subsequent one doesn't wait yet-another two animation frames-- I'll tell you what I'll make an issue to add more cases to the layout complete tests to verify expectations around cascading resize effects and the layoutComplete promise as a separate issue and approve this as is.

Yeah, a full set of layoutComplete tests would be great. I think any given cycle of child measurements and re-layouts is going to occur within a single frame, so believe this logic is ok—but we should certainly confirm via tests.

* add test for lit#3518 * add fix for lit#3518 * add test for lit#3481 * add fix for lit#3481

Gray Norton and others added 4 commits December 13, 2022 09:53

add test for #3518

af1838d

add fix for #3518

51daa9b

add test for #3481

e45488c

add fix for #3481

da8eb5f

graynorton requested a review from usergenic December 13, 2022 17:55

graynorton requested a review from justinfagnani as a code owner December 13, 2022 17:55

usergenic approved these changes Dec 15, 2022

View reviewed changes

graynorton merged commit 393e30c into main Dec 15, 2022

graynorton deleted the fix-3481 branch December 15, 2022 04:29

This was referenced Dec 15, 2022

Version Packages #3528

Closed

Version Packages #3531

Merged

43081j pushed a commit to 43081j/lit-html that referenced this pull request Jan 4, 2023

[labs/virtualizer] Fix lit#3481 and lit#3518 (lit#3519)

365dc6e

* add test for lit#3518 * add fix for lit#3518 * add test for lit#3481 * add fix for lit#3481

Conversation

graynorton commented Dec 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Dec 13, 2022

🦋 Changeset detected

Uh oh!

github-actions bot commented Dec 13, 2022 • 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!

usergenic left a comment

Choose a reason for hiding this comment

Uh oh!

usergenic Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graynorton Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

graynorton commented Dec 13, 2022 •

edited

Loading

github-actions bot commented Dec 13, 2022 •

edited

Loading

usergenic Dec 15, 2022 •

edited

Loading