core(cls-all-frames): use weighted score from trace #12034

adamraine · 2021-02-01T21:13:13Z

Closes #12023

CLS-AF will be calculated with weighted_score_delta instead of score.

I'm not planning on merging until after weighted score is merged in M89:
https://bugs.chromium.org/p/chromium/issues/detail?id=1173139
Patch is not getting merged into M89, not waiting for M90 to merge this.

adamraine · 2021-02-01T21:15:15Z

lighthouse-core/test/computed/metrics/timing-summary-test.js

-const trace = require('../../fixtures/traces/frame-metrics-m89.json');
-const devtoolsLog = require('../../fixtures/traces/frame-metrics-m89.devtools.log.json');
+const trace = require('../../fixtures/traces/frame-metrics-m90.json');
+const devtoolsLog = require('../../fixtures/traces/frame-metrics-m90.devtools.log.json');


I tried to avoid changing our expectations for LCP-AF and FCP-AF in any tests that I could. This is the only place I couldn't.

adamraine · 2021-02-01T21:17:21Z

lighthouse-core/test/fixtures/traces/frame-metrics-m90.json

@@ -0,0 +1,242 @@
+{


Keeping frame-metrics-m89.json around so we don't have to change expectations for LCP-AF and FCP-AF everywhere.

adamraine · 2021-02-01T21:29:13Z

lighthouse-core/test/audits/__snapshots__/metrics-test.js.snap

@@ -3,7 +3,7 @@
 exports[`Performance: metrics evaluates valid input (with lcp from all frames) correctly 1`] = `
 Object {
  "cumulativeLayoutShift": 0.0011656245471340055,
-  "cumulativeLayoutShiftAllFrames": 0.4591700003057729,
+  "cumulativeLayoutShiftAllFrames": undefined,


This snapshot is using the old trace without the weighted score, so CLS-AF is undefined.

patrickhulce

LGTM just the throw decision outstanding I think :)

patrickhulce · 2021-02-01T21:25:45Z

lighthouse-core/computed/metrics/cumulative-layout-shift-all-frames.js

+   * @param {LH.TraceEvent} event
+   * @return {event is LayoutShiftEvent}
+   */
+  static isLayoutShiftEvent(event) {


nit: isValidLayoutShiftEvent or something? I wouldn't expect all of the validation and throw logic in a simple check

I'm leaving this as is because the validation and throw logic are removed.

patrickhulce · 2021-02-01T21:26:00Z

lighthouse-core/computed/metrics/cumulative-layout-shift-all-frames.js

+    // Weighted score was added to the trace in m89:
+    // https://bugs.chromium.org/p/chromium/issues/detail?id=1173139
+    if (typeof event.args.data.weighted_score_delta !== 'number') {
+      throw new LHError(


should this fallback to score? seems like there's still some value in seeing if the all frames version and main-frame version are different. I understand the difficulty of figuring out which CLS values are valid, but seems like we're going to have to do that anyway with a mix of LH versions in the same month.

I'm good with doing a fallback and replacing this with a log warning for now.

I'm just worried that the fallback doesn't make sense in the long run. Once we get through the churn of different LH and Chrome versions I would like to remove the fallback.

SG, can throw a FIXME: remove after XX on there :)

lighthouse-core/test/computed/metrics/cumulative-layout-shift-all-frames-test.js

lighthouse-core/test/audits/metrics-test.js

adamraine · 2021-02-02T17:50:19Z

@patrickhulce sorry to increase the scope a bit, but I noticed another pretty big error while picking through some HTTPArchive results. Many of the sites had a CLS-AF smaller than CLS which doesn't make sense since extra frames should only add to CLS.

Turns out I didn't add the correct had_recent_input logic from the main frame CLS:

lighthouse/lighthouse-core/computed/metrics/cumulative-layout-shift.js

Lines 52 to 65 in 5c32e64

    
           // Chromium will set `had_recent_input` if there was recent user input, which 
        
           // skips shift events from contributing to CLS. This flag is also set when Lighthouse changes 
        
           // the emulation size. This consistently results in the first few shift event always being 
        
           // ignored for CLS. Since we don't expect any user input, we add the score of these 
        
           // shift events to CLS. 
        
           // See https://bugs.chromium.org/p/chromium/issues/detail?id=1094974. 
        
           for (let i = 0; i < traceOfTab.mainThreadEvents.length; i++) { 
        
             const evt = traceOfTab.mainThreadEvents[i]; 
        
             if (evt.name === 'LayoutShift' && evt.args && evt.args.data && evt.args.data.is_main_frame) { 
        
               if (!evt.args.data.had_recent_input) break; 
        
               if (typeof evt.args.data.score !== 'number') continue; 
        
               cumulativeLayoutShift += evt.args.data.score; 
        
             } 
        
           }

I added changes in this PR to address this error.

connorjclark · 2021-02-02T18:06:36Z

lighthouse-core/computed/metrics/cumulative-layout-shift-all-frames.js

+    // ignored for CLS. Since we don't expect any user input, we add the score of these
+    // shift events to CLS.
+    // See https://bugs.chromium.org/p/chromium/issues/detail?id=1094974.
+    for (const event of layoutShiftEvents) {


Some more comments here would be useful. Is this accurate?

We set the first initial events that appear as had_recent_input=true to false, up until the first time had_recent_input=false appears. This ignores the incorrect inputs that are an artifact of Lighthouse changing the viewport size, and keeps any inputs that appear later in the page load (as a result of a puppeteer script controlling the page).

does the logic here need to be more complex now in a multi-frame world? how does had_recent_input affect subframes when their viewports are resized from js? should those actually be counted? if so, the "had_recent_input" name is really starting to stretch 😆

+1 to everything above, plus should we be doing this fixup in trace-of-tab (well, trace-processor, actually)? Or not altering the original trace events and doing it on a copy of the LayoutShift events in each of these audits...otherwise the state of early LayoutShift events depends on what other audits/computed artifacts have already run (e.g. an ads plugin CLS audit that either has to know about this issue or has to only run after the perf audits to get the numbers right)

connorjclark · 2021-02-02T18:07:27Z

lighthouse-core/computed/metrics/cumulative-layout-shift-all-frames.js

-        return /** @type {number} */ (e.args.data.score);
+        if (e.args.data.had_recent_input) return 0;
+
+        // FIXME: remove after 89 hits stable


@paulirish likes COMPAT

yeah I guess this is more COMPAT than FIXME, sorry for the bad suggestion @adamraine

patrickhulce

Nice catch I completely forgot about that workaround too! We should probably share the computation of CLS based on a sequence of layoutshift events now that it's sufficiently complicated though. How disappointed would you be to move that fix and a sharing refactor to a new PR? Anytime there's logic requiring a comment that large, feels like it shouldn't be duplicated :)

Unrelated but important to note that we won't really have a similar workaround ability for FR :/ we'll cross that bridge when we get there I suppose.

patrickhulce · 2021-02-02T18:12:56Z

lighthouse-core/computed/metrics/cumulative-layout-shift-all-frames.js

+    // ignored for CLS. Since we don't expect any user input, we add the score of these
+    // shift events to CLS.
+    // See https://bugs.chromium.org/p/chromium/issues/detail?id=1094974.
+    for (const event of layoutShiftEvents) {


does the logic here need to be more complex now in a multi-frame world? how does had_recent_input affect subframes when their viewports are resized from js? should those actually be counted? if so, the "had_recent_input" name is really starting to stretch 😆

adamraine · 2021-02-02T18:54:31Z

Ok, moving the had_recent_input logic to a separate PR.

adamraine · 2021-02-02T19:10:57Z

Another update: release manager would prefer the weighted score change stay in M90:
https://bugs.chromium.org/p/chromium/issues/detail?id=1173139#c4

I don't think this is a major issue for us, just means we need to adjust some comments and stuff here.

patrickhulce

still LGTM thanks for punting the other input fix :)

lighthouse-core/test/computed/metrics/cumulative-layout-shift-all-frames-test.js

adamraine added 2 commits February 1, 2021 15:59

a

3f0b627

smoke version

8efaa60

adamraine requested a review from a team as a code owner February 1, 2021 21:13

adamraine requested review from patrickhulce and removed request for a team February 1, 2021 21:13

google-cla bot added the cla: yes label Feb 1, 2021

devtools-bot assigned patrickhulce Feb 1, 2021

devtools-bot added the waiting4reviewer label Feb 1, 2021

adamraine commented Feb 1, 2021

View reviewed changes

score

1eea8bd

vercel bot deployed to Preview February 1, 2021 21:33 View deployment

patrickhulce approved these changes Feb 1, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Feb 1, 2021

adamraine added 2 commits February 2, 2021 11:57

comments

8cc358e

fix

e53d307

vercel bot deployed to Preview February 2, 2021 17:37 View deployment

adamraine added waiting4reviewer and removed waiting4committer labels Feb 2, 2021

connorjclark reviewed Feb 2, 2021

View reviewed changes

patrickhulce reviewed Feb 2, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Feb 2, 2021

compat

dc73219

move logic

0d7b584

vercel bot deployed to Preview February 2, 2021 19:05 View deployment

update snapshot

18d0ed9

vercel bot deployed to Preview February 2, 2021 19:22 View deployment

90

5bd611d

vercel bot deployed to Preview February 2, 2021 19:25 View deployment

adamraine added waiting4reviewer and removed waiting4committer labels Feb 2, 2021

patrickhulce approved these changes Feb 3, 2021

View reviewed changes

lighthouse-core/test/computed/metrics/cumulative-layout-shift-all-frames-test.js Outdated Show resolved Hide resolved

patrickhulce added waiting4committer and removed waiting4reviewer labels Feb 3, 2021

rm

da8cb8c

vercel bot deployed to Preview February 3, 2021 17:33 View deployment

adamraine added land-when-ci-is-green and removed waiting4committer labels Feb 3, 2021

devtools-bot merged commit 315c367 into master Feb 3, 2021

devtools-bot deleted the cls-af-weighted branch February 3, 2021 20:33

devtools-bot removed the land-when-ci-is-green label Feb 3, 2021

brendankenny mentioned this pull request Feb 9, 2021

core: add hidden layout-shift variant metrics #12046

Merged

adamraine mentioned this pull request Feb 9, 2021

core(cls): refactor CLS and CLS-AF #12069

Merged

paulirish pushed a commit that referenced this pull request Mar 23, 2021

core(cls-all-frames): use weighted score from trace (#12034)

0576e29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(cls-all-frames): use weighted score from trace #12034

core(cls-all-frames): use weighted score from trace #12034

adamraine commented Feb 1, 2021 •

edited

Loading

adamraine Feb 1, 2021

adamraine Feb 1, 2021

adamraine Feb 1, 2021

patrickhulce left a comment

patrickhulce Feb 1, 2021

adamraine Feb 2, 2021

patrickhulce Feb 1, 2021

adamraine Feb 1, 2021

patrickhulce Feb 2, 2021

adamraine commented Feb 2, 2021

connorjclark Feb 2, 2021 •

edited

Loading

patrickhulce Feb 2, 2021

brendankenny Feb 2, 2021 •

edited

Loading

connorjclark Feb 2, 2021

patrickhulce Feb 2, 2021 •

edited

Loading

patrickhulce left a comment

patrickhulce Feb 2, 2021

adamraine commented Feb 2, 2021

adamraine commented Feb 2, 2021

patrickhulce left a comment

core(cls-all-frames): use weighted score from trace #12034

core(cls-all-frames): use weighted score from trace #12034

Conversation

adamraine commented Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Feb 2, 2021

connorjclark Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Feb 2, 2021 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamraine commented Feb 2, 2021

adamraine commented Feb 2, 2021

patrickhulce left a comment

Choose a reason for hiding this comment

adamraine commented Feb 1, 2021 •

edited

Loading

connorjclark Feb 2, 2021 •

edited

Loading

brendankenny Feb 2, 2021 •

edited

Loading

patrickhulce Feb 2, 2021 •

edited

Loading