Don't clone nodes as part of the debug event logger #2635

rictic · 2022-03-14T22:56:08Z

Integrations that need to clone nodes can do so themselves

Fixes #2632

Writing it this way will do even less work (and no side effects) even in dev mode, unless the page has opted in.

changeset-bot · 2022-03-14T22:56:11Z

🦋 Changeset detected

Latest commit: 18a0074

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-14T22:56:34Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -1% - +5% (-0.26ms - +1.52ms)
^{this-change vs tip-of-tree}

render

lit-element-list: unsure 🔍 -2% - +1% (-1.61ms - +0.84ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -1% - +3% (-0.24ms - +0.92ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +2% (-0.36ms - +0.20ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +1% (-1.56ms - +0.42ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +2% (-0.63ms - +1.38ms)
^{this-change vs tip-of-tree}

update

lit-element-list: unsure 🔍 -1% - +1% (-4.87ms - +10.24ms)
^{this-change vs tip-of-tree}
lit-html-kitchen-sink: unsure 🔍 -3% - +3% (-2.79ms - +2.93ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -0% - +2% (-1.68ms - +7.34ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +2% (-1.69ms - +2.25ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +2% (-3.77ms - +15.10ms)
^{this-change vs tip-of-tree}

update-reflect

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

Results

lit-element-list

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2511
Commit: ae71dae

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	85.62ms - 87.32ms	-	unsure 🔍 -2% - +1% -1.61ms - +0.84ms	faster ✔ 19% - 21% 21.01ms - 23.35ms
tip-of-tree tip-of-tree	85.97ms - 87.74ms	unsure 🔍 -1% - +2% -0.84ms - +1.61ms	-	faster ✔ 19% - 21% 20.60ms - 22.99ms
previous-release previous-release	107.84ms - 109.45ms	slower ❌ 24% - 27% 21.01ms - 23.35ms	slower ❌ 24% - 27% 20.60ms - 22.99ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	868.63ms - 878.71ms	-	unsure 🔍 -1% - +1% -4.87ms - +10.24ms	faster ✔ 7% - 8% 62.30ms - 77.66ms
tip-of-tree tip-of-tree	865.35ms - 876.61ms	unsure 🔍 -1% - +1% -10.24ms - +4.87ms	-	faster ✔ 7% - 9% 64.59ms - 80.75ms
previous-release previous-release	937.85ms - 949.45ms	slower ❌ 7% - 9% 62.30ms - 77.66ms	slower ❌ 7% - 9% 64.59ms - 80.75ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	957.05ms - 969.63ms	-	unsure 🔍 -1% - +1% -10.95ms - +6.33ms	faster ✔ 4% - 6% 39.57ms - 56.29ms
tip-of-tree tip-of-tree	959.73ms - 971.57ms	unsure 🔍 -1% - +1% -6.33ms - +10.95ms	-	faster ✔ 4% - 5% 37.52ms - 53.71ms
previous-release previous-release	1005.75ms - 1016.78ms	slower ❌ 4% - 6% 39.57ms - 56.29ms	slower ❌ 4% - 6% 37.52ms - 53.71ms	-

lit-html-kitchen-sink

Browser: chrome-headless 98.0.4758.102
Sample size: 30
Built by: Benchmarks #2511
Commit: ae71dae

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	34.14ms - 35.14ms	-	unsure 🔍 -1% - +3% -0.24ms - +0.92ms	faster ✔ 14% - 19% 5.61ms - 7.85ms
tip-of-tree tip-of-tree	34.01ms - 34.58ms	unsure 🔍 -3% - +1% -0.92ms - +0.24ms	-	faster ✔ 15% - 19% 6.04ms - 8.12ms
previous-release previous-release	40.37ms - 42.37ms	slower ❌ 16% - 23% 5.61ms - 7.85ms	slower ❌ 18% - 24% 6.04ms - 8.12ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	93.81ms - 98.15ms	-	unsure 🔍 -3% - +3% -2.79ms - +2.93ms	faster ✔ 3% - 10% 3.13ms - 10.24ms
tip-of-tree tip-of-tree	94.04ms - 97.78ms	unsure 🔍 -3% - +3% -2.93ms - +2.79ms	-	faster ✔ 3% - 10% 3.37ms - 10.14ms
previous-release previous-release	99.84ms - 105.48ms	slower ❌ 3% - 11% 3.13ms - 10.24ms	slower ❌ 3% - 11% 3.37ms - 10.14ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	30.40ms - 31.99ms	-	unsure 🔍 -1% - +5% -0.26ms - +1.52ms	faster ✔ 9% - 16% 3.15ms - 5.76ms
tip-of-tree tip-of-tree	30.16ms - 30.97ms	unsure 🔍 -5% - +1% -1.52ms - +0.26ms	-	faster ✔ 12% - 17% 3.98ms - 6.20ms
previous-release previous-release	34.61ms - 36.69ms	slower ❌ 10% - 19% 3.15ms - 5.76ms	slower ❌ 13% - 20% 3.98ms - 6.20ms	-

lit-html-repeat

Browser: chrome-headless 98.0.4758.102
Sample size: 30
Built by: Benchmarks #2511
Commit: ae71dae

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.50ms - 12.92ms	-	unsure 🔍 -3% - +2% -0.36ms - +0.20ms	faster ✔ 11% - 18% 1.61ms - 2.67ms
tip-of-tree tip-of-tree	12.60ms - 12.98ms	unsure 🔍 -2% - +3% -0.20ms - +0.36ms	-	faster ✔ 11% - 17% 1.54ms - 2.58ms
previous-release previous-release	14.37ms - 15.33ms	slower ❌ 13% - 21% 1.61ms - 2.67ms	slower ❌ 12% - 20% 1.54ms - 2.58ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	354.65ms - 361.96ms	-	unsure 🔍 -0% - +2% -1.68ms - +7.34ms	faster ✔ 30% - 31% 152.02ms - 163.34ms
tip-of-tree tip-of-tree	352.83ms - 358.12ms	unsure 🔍 -2% - +0% -7.34ms - +1.68ms	-	faster ✔ 30% - 32% 155.44ms - 165.57ms
previous-release previous-release	511.66ms - 520.30ms	slower ❌ 42% - 46% 152.02ms - 163.34ms	slower ❌ 44% - 47% 155.44ms - 165.57ms	-

lit-html-template-heavy

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2511
Commit: ae71dae

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	62.89ms - 64.12ms	-	unsure 🔍 -2% - +1% -1.56ms - +0.42ms	faster ✔ 16% - 19% 11.81ms - 14.35ms
tip-of-tree tip-of-tree	63.29ms - 64.85ms	unsure 🔍 -1% - +2% -0.42ms - +1.56ms	-	faster ✔ 15% - 18% 11.16ms - 13.87ms
previous-release previous-release	75.47ms - 77.69ms	slower ❌ 18% - 23% 11.81ms - 14.35ms	slower ❌ 17% - 22% 11.16ms - 13.87ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	137.21ms - 140.17ms	-	unsure 🔍 -1% - +2% -1.69ms - +2.25ms	faster ✔ 12% - 15% 19.50ms - 24.62ms
tip-of-tree tip-of-tree	137.11ms - 139.71ms	unsure 🔍 -2% - +1% -2.25ms - +1.69ms	-	faster ✔ 13% - 15% 19.88ms - 24.79ms
previous-release previous-release	158.66ms - 162.84ms	slower ❌ 14% - 18% 19.50ms - 24.62ms	slower ❌ 14% - 18% 19.88ms - 24.79ms	-

reactive-element-list

Browser: chrome-headless 98.0.4758.102
Sample size: 50
Built by: Benchmarks #2511
Commit: ae71dae

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	62.08ms - 63.51ms	-	unsure 🔍 -1% - +2% -0.63ms - +1.38ms	unsure 🔍 -2% - +2% -1.10ms - +1.28ms
tip-of-tree tip-of-tree	61.71ms - 63.13ms	unsure 🔍 -2% - +1% -1.38ms - +0.63ms	-	unsure 🔍 -2% - +1% -1.46ms - +0.90ms
previous-release previous-release	61.75ms - 63.65ms	unsure 🔍 -2% - +2% -1.28ms - +1.10ms	unsure 🔍 -1% - +2% -0.90ms - +1.46ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	894.90ms - 907.04ms	-	unsure 🔍 -0% - +2% -3.77ms - +15.10ms	unsure 🔍 -0% - +2% -1.90ms - +14.57ms
tip-of-tree tip-of-tree	888.08ms - 902.53ms	unsure 🔍 -2% - +0% -15.10ms - +3.77ms	-	unsure 🔍 -1% - +1% -8.45ms - +9.79ms
previous-release previous-release	889.07ms - 900.20ms	unsure 🔍 -2% - +0% -14.57ms - +1.90ms	unsure 🔍 -1% - +1% -9.79ms - +8.45ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	1004.65ms - 1019.17ms	-	unsure 🔍 -1% - +1% -10.31ms - +9.16ms	unsure 🔍 -1% - +1% -8.00ms - +12.02ms
tip-of-tree tip-of-tree	1006.00ms - 1018.97ms	unsure 🔍 -1% - +1% -9.16ms - +10.31ms	-	unsure 🔍 -1% - +1% -6.87ms - +12.05ms
previous-release previous-release	1003.01ms - 1016.79ms	unsure 🔍 -1% - +1% -12.02ms - +8.00ms	unsure 🔍 -1% - +1% -12.05ms - +6.87ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2022-03-15T00:13:41Z

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

      const shouldEmit = (window as unknown as DebugLoggingWindow)
        .emitLitDebugLogEvents;
      if (shouldEmit) {
+        return undefined;


Should the condition here be inverted?

Eep, yes! I even got this right in the other location.

justinfagnani · 2022-03-15T00:16:27Z

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

      const shouldEmit = (window as unknown as DebugLoggingWindow)
        .emitLitDebugLogEvents;
      if (shouldEmit) {
+        return undefined;
+      }
+      return (event: LitUnstable.DebugLog.Entry) => {


Do you think it's that much better to be lazier here rather than removing the clone() calls? Why does the element need to be cloned in the first place? I think that could still fail in some cases when the event are enabled.

Hm, yeah, good point, any event log subscribers that want copies of previous committed values would still synchronously get access to those values, so they could do the clone themselves if necessary

Make the event debug logger lazier

5972710

Writing it this way will do even less work (and no side effects) even in dev mode, unless the page has opted in.

rictic requested a review from justinfagnani as a code owner March 14, 2022 22:56

justinfagnani reviewed Mar 15, 2022

View reviewed changes

rictic added 2 commits March 15, 2022 08:17

Fix conditional

be2c77e

Just avoid the clone call, a lot of laziness isn't necessary here

18a0074

rictic changed the title ~~Make the event debug logger lazier~~ Don't clone nodes as part of the debug event logger Mar 15, 2022

justinfagnani approved these changes Mar 15, 2022

View reviewed changes

rictic merged commit ae35870 into main Mar 15, 2022

rictic deleted the lazier-debug-events branch March 15, 2022 16:10

lit-robot mentioned this pull request Mar 15, 2022

Version Packages #2639

Closed

aomarks added a commit that referenced this pull request Mar 15, 2022

Add Lit to changeset for #2635

8fcd4ff

aomarks mentioned this pull request Mar 15, 2022

Add Lit to changeset for #2635 #2640

Merged

aomarks added a commit that referenced this pull request Mar 15, 2022

Add Lit to changeset for #2635 (#2640)

18b0ca6

lit-robot mentioned this pull request Mar 15, 2022

Version Packages #2641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't clone nodes as part of the debug event logger #2635

Don't clone nodes as part of the debug event logger #2635

rictic commented Mar 14, 2022 •

edited

changeset-bot bot commented Mar 14, 2022 •

edited

github-actions bot commented Mar 14, 2022 •

edited

📊 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

justinfagnani Mar 15, 2022

rictic Mar 15, 2022

justinfagnani Mar 15, 2022

rictic Mar 15, 2022 •

edited

Don't clone nodes as part of the debug event logger #2635

Don't clone nodes as part of the debug event logger #2635

Conversation

rictic commented Mar 14, 2022 • edited

changeset-bot bot commented Mar 14, 2022 • edited

🦋 Changeset detected

github-actions bot commented Mar 14, 2022 • edited

📊 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

justinfagnani Mar 15, 2022

Choose a reason for hiding this comment

rictic Mar 15, 2022

Choose a reason for hiding this comment

justinfagnani Mar 15, 2022

Choose a reason for hiding this comment

rictic Mar 15, 2022 • edited

Choose a reason for hiding this comment

rictic commented Mar 14, 2022 •

edited

changeset-bot bot commented Mar 14, 2022 •

edited

github-actions bot commented Mar 14, 2022 •

edited

rictic Mar 15, 2022 •

edited