core: add hidden layout-shift variant metrics #12046

brendankenny · 2021-02-04T00:03:13Z

Adds the ("normalizing") Layout Shift variants to CLS described in "Feedback wanted: The road to a better layout shift metric for long-lived pages" to the hidden metrics audit.

Landing soonish (and doing a release) would allow us to start collecting data over many sites using WPT throttling via HTTP Archive and using simulated throttling through local/cloud testing.

Areas of interest:

variants vs CLS in lab testing (some speculation that since the window is already limited by load time there may be minimal impact for Lighthouse when switching LayoutShift metrics)
relative merits of the different variants
effect of simulated vs applied throttling (do we need some level of Lantern support for the windowed LayoutShift variants?)
acquire a baseline of data for each variant so if/when one is chosen for a scored metric we'll be able to pick curves and model the effect on scores

Implementation is almost entirely from https://github.com/mmocny/web-vitals/wiki/Snippets-for-LSN-using-PerformanceObserver. Further abstraction/generalization is probably not worth it since we won't be iterating on these in Lighthouse and we expect the multiple-variant implementation to be temporary.

patrickhulce

neat!

some speculation that since the window is already limited by load time there may be minimal impact for Lighthouse when switching LayoutShift metrics

I have a strong suspicion of this as well, at least for the longer windowed variants (and the tests seems to agree 😉 ). Getting this into some FR timespan testing is another story though, big game changer there :D

effect of simulated vs applied throttling (do we need some level of Lantern support for the windowed LayoutShift variants?)

Do we have reason to suspect this will be any different than for CLS? AFAIK, Lantern CLS is almost always matching or underreporting real CLS and it's essentially impossible to predict the magnitude of the shifts would have occurred when network is stretched out in any principled way.

patrickhulce · 2021-02-04T01:03:06Z

lighthouse-core/computed/layout-shift-variants.js

+   * @param {LH.Artifacts.TraceOfTab} traceOfTab
+   * @return {Array<LayoutShiftEvent>}
+   */
+  static getLayoutShiftEvents(traceOfTab) {


cc @adamraine maybe there's interplay between your planned refactor and this PR? seems like CLS itself could be a "layout shift variant" maxSession(allEvents, Infinity, Infinity)

heck our current impl should probably use this :)

My plan for the refractor was to add the layout shift events for all frames to TraceOfTab and then filter/aggregate the events in CLS and CLS-AF. The had_recent_input logic would be in TraceProcessor.

I could use this getLayoutShiftEvents instead if it wasn't specific to main frame events. Maybe pick out the main frame events in _compute? It would also make sense to move this function elsewhere since it wouldn't be specific to this file.

Either way, we should probably be refactoring main frame CLS so we don't repeat the had_recent_input logic.

I could use this getLayoutShiftEvents instead if it wasn't specific to main frame events. Maybe pick out the main frame events in _compute?

yeah, that would work fine

My plan for the refractor was to add the layout shift events for all frames to TraceOfTab and then filter/aggregate the events in CLS and CLS-AF. The had_recent_input logic would be in TraceProcessor.

@patrickhulce are we just making our job harder for FR, though? I wonder if any correction we do should be pushed back even further...like should the correction be done during gathering, when everything knows whether or not it was a navigation and so can confidently correct had_recent_input?

OTOH for timeOrigin stuff, TraceProcessor is going to have to be given some level of knowledge about what kind of run the trace comes from anyways...

(edit: or we just share a getLayoutShiftEvents function between the various CLS impls and don't try to correct the trace at all)

are we just making our job harder for FR, though? I wonder if any correction we do should be pushed back even further...like should the correction be done during gathering, when everything knows whether or not it was a navigation and so can confidently correct had_recent_input?

When we get there, the workaround will need to be an option for sure. Unfortunately, I think we're just kinda screwed for FR no matter what we do. I called this out in @adamraine's PR too that if we're auto enabling emulation with timespans we'll be stuck in between both of these (we want to ignore had_recent_input for the emulation change just like navigation but the user could have had real input too :/)

Either way, I'm fine with whatever is most comfortable for these two immediate PRs because FR trace processing is a massive project I don't expect to be significantly affected by this decision here.

edit: or we just share a getLayoutShiftEvents function between the various CLS impls and don't try to correct the trace at all

This was roughly what I had in mind originally but with options: {ignoreInitialHadRecentInputSequence: boolean} or something

Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.

Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.

this is duplicating but based on #12034 (comment) (and the conversation before it) it sounded like we still need a third copy and that'll be the time to do the refactor for them to all share some level of implementation. cc @adamraine

I can also do the follow up refactor if there's too much else going on right now.

it sounded like we still need a third copy and that'll be the time to do the refactor for them to all share some level of implementation

Yeah, I think we should settle the duplication in a follow-up.

I can also do the follow up refactor if there's too much else going on right now.

I started working on this locally, but I was waiting for this to land so I could use getLayoutShiftEvents.

patrickhulce · 2021-02-04T01:03:34Z

lighthouse-core/computed/layout-shift-variants.js

+    for (const event of layoutShiftEvents) {
+      if (event.ts - prevTs > 5_000_000) clusterCount++;
+      prevTs = event.ts;
+      score += event.score;


are we ignoring framehole here for now?

are we ignoring framehole here for now?

yes, see the // Main frame only for now comment below. Since cumulativeLayoutShiftAllFrames is currently in the data gathering stage it seemed like it made the most sense to make the direct comparison to main-frame-only CLS. We could add all frame versions of these variants if it made sense, though?

I'd like to include the allFrames variants as part of this yeah.

I'd like to include the allFrames variants as part of this yeah.

With the size this PR has grown, would you be ok with a follow-up?

(With #12023 it sounds like we can't do it right until m90, which won't be used by HTTP archive until April (?), so we could maybe wait for @adamraine's refactor as well if that's incoming)

I'd like to include the allFrames variants as part of this yeah.

With the size this PR has grown, would you be ok with a follow-up?

sure

patrickhulce · 2021-02-04T01:07:02Z

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

+  it('evaluates LayoutShift variants correctly', async () => {
+    const artifacts = {
+      traces: {
+        [MetricsAudit.DEFAULT_PASS]: clsAllFramesTrace,


do we have a real-world test for these where they aren't all identical? if we don't know of any test sites, something that layoutshifts in a set pattern on glitch or something might be just as quick to spin up

do we have a real-world test for these where they aren't all identical? if we don't know of any test sites, something that layoutshifts in a set pattern on glitch or something might be just as quick to spin up

no, closest is frame-metrics-m89/frame-metrics-m90, but those both only have a single event in the main frame (the other 25 events in frame-metrics-m89 are in other frames). I didn't really want to add another trace to our menagerie but I guess it's for the best :)

added a trace

lighthouse-core/test/computed/layout-shift-variants-test.js

brendankenny · 2021-02-04T01:39:01Z

Do we have reason to suspect this will be any different than for CLS?

No, pure--possibly not helpful--speculation :)

brendankenny · 2021-02-04T21:20:56Z

also added tests for boundary conditions, because those are easy to mess up in refactors (e.g. inverting the condition but not correcting for inclusive/exclusive) and not notice. Verified the results with the Speed Metrics tool and that they fail if any of the conditionals are switched from > to >=.

patrickhulce

LGTM

patrickhulce · 2021-02-08T18:50:45Z

lighthouse-core/computed/layout-shift-variants.js

+   * @param {LH.Artifacts.TraceOfTab} traceOfTab
+   * @return {Array<LayoutShiftEvent>}
+   */
+  static getLayoutShiftEvents(traceOfTab) {


Was this settled? It seems like we're still just duplicating at the moment, and I'm a little unclear of how it will be resolved in a followup.

brendankenny requested a review from a team as a code owner February 4, 2021 00:03

brendankenny requested review from patrickhulce and removed request for a team February 4, 2021 00:03

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

devtools-bot assigned patrickhulce Feb 4, 2021

devtools-bot added the waiting4reviewer label Feb 4, 2021

core: add hidden layout-shift variant metrics

e9d6d49

brendankenny force-pushed the lsvariants branch from 1c392b6 to e9d6d49 Compare February 4, 2021 00:08

vercel bot deployed to Preview February 4, 2021 00:08 View deployment

fix merge error

024e2f8

vercel bot deployed to Preview February 4, 2021 00:25 View deployment

patrickhulce reviewed Feb 4, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Feb 4, 2021

brendankenny added 3 commits February 4, 2021 10:50

update tests

669c0d2

add jumpy cls trace

7129ed1

add boundary tests

8fa4c09

vercel bot deployed to Preview February 4, 2021 21:15 View deployment

brendankenny added waiting4reviewer and removed waiting4committer labels Feb 4, 2021

patrickhulce approved these changes Feb 8, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Feb 8, 2021

adamraine mentioned this pull request Feb 9, 2021

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

Merged

brendankenny merged commit 7cba5fe into master Feb 9, 2021

brendankenny deleted the lsvariants branch February 9, 2021 21:18

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

core: add hidden layout-shift variant metrics (#12046)

9a15965

paulirish mentioned this pull request Apr 9, 2021

Update CLS definition #12350

Closed

7 tasks

brendankenny mentioned this pull request May 13, 2021

core: add new CLS (all frames) to hidden metrics audit #12476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add hidden layout-shift variant metrics #12046

core: add hidden layout-shift variant metrics #12046

brendankenny commented Feb 4, 2021

patrickhulce left a comment

patrickhulce Feb 4, 2021

adamraine Feb 4, 2021

brendankenny Feb 4, 2021 •

edited

Loading

patrickhulce Feb 4, 2021

patrickhulce Feb 8, 2021

brendankenny Feb 9, 2021

brendankenny Feb 9, 2021

adamraine Feb 9, 2021

adamraine Feb 9, 2021

patrickhulce Feb 4, 2021

brendankenny Feb 4, 2021

paulirish Feb 4, 2021

brendankenny Feb 5, 2021

paulirish Feb 8, 2021

patrickhulce Feb 4, 2021

brendankenny Feb 4, 2021

brendankenny Feb 4, 2021

brendankenny commented Feb 4, 2021

brendankenny commented Feb 4, 2021 •

edited

Loading

patrickhulce left a comment

patrickhulce Feb 8, 2021

core: add hidden layout-shift variant metrics #12046

core: add hidden layout-shift variant metrics #12046

Conversation

brendankenny commented Feb 4, 2021

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

brendankenny Feb 4, 2021 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Feb 4, 2021

brendankenny commented Feb 4, 2021 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Feb 4, 2021 •

edited

Loading

brendankenny commented Feb 4, 2021 •

edited

Loading