Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new_audit: add cumulative layout shift metric #9037

Merged
merged 42 commits into from
Feb 28, 2020
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented May 23, 2019

This lands CLS as an invisible metric

image

Scoring is uncalibrated still, as is determining how we surface a number between 0 and 2.0-ish to the user. :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excited to play around with the results of this!


return {
score: Audit.computeLogNormalScore(
metricResult.timing,

This comment was marked as resolved.

const MetricArtifact = require('./metric');
const LHError = require('../../lib/lh-error');

class LayoutStability extends MetricArtifact {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably don't need to bother with extending metricartifact since there's no difference between lantern and observed. it's only useful when we need to utilize the graph and account for both modes.

this should also help eliminate the .timing issue

This comment was marked as resolved.

@@ -103,6 +103,9 @@ class Driver {
// Used instead of 'toplevel' in Chrome 71+
'disabled-by-default-lighthouse',

// Used for Layout Stability metric
'loading',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how beefy is this category?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3% on a verge trace. not bad

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why you hoarding this script paul

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* eslint-disable */
'use strict';

const trace = require('../latest-run/defaultPass.trace.json');
const {traceCategories} = require('../lighthouse-core/gather/driver');

const traceCats = {};
// aggregate
let totalBytes = 0;
let totalEvents = 0;

trace.traceEvents.forEach(e => {
  let eventCats = e.cat;
  for (let eventCat of eventCats.split(',')) {
    // don't process cats we dont trace
    // if (!traceCategories.includes(eventCat)) return;
    if (e.name === 'ThreadControllerImpl::RunTask') eventCat += '::::::::RunTask';
    const cat = traceCats[eventCat] || {bytes: 0, events: 0};
    const bytes = JSON.stringify(e).length;
    cat.bytes += bytes;
    totalBytes += bytes;
    cat.events += 1;
    totalEvents += 1;
    traceCats[eventCat] = cat;
  }
});




// obj to array
const traceTotals = [];
Object.keys(traceCats).forEach(catname => {
	const cat = traceCats[catname];
	traceTotals.push({name: catname, bytes: cat.bytes, events: cat.events});
});

// sort and log
console.log('Bytes'.padStart(16), '\t', 'Count'.padStart(7), '\t', 'Event Name'.padStart(18))
traceTotals.sort((a, b) => b.bytes - a.bytes).forEach((tot, i) => {
	console.log(
    tot.bytes.toLocaleString().padStart(15), 
    `${(tot.bytes * 100/ totalBytes).toLocaleString(undefined, {maximumFractionDigits: 1})}%`.padStart(6),
    '\t', 
    tot.events.toString().padStart(9), 
    `${(tot.events * 100/ totalEvents).toLocaleString(undefined, {maximumFractionDigits: 1})}%`.padStart(6),
    '\t', 
    tot.name
  );
})

@@ -236,6 +236,12 @@ const ERRORS = {
lhrRuntimeError: true,
},

// No Layout Stability trace events
NO_LAYOUT_JANK: {

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member Author

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

been a while. its ready for a look!

@@ -103,6 +103,9 @@ class Driver {
// Used instead of 'toplevel' in Chrome 71+
'disabled-by-default-lighthouse',

// Used for Layout Stability metric
'loading',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3% on a verge trace. not bad

image

const MetricArtifact = require('./metric');
const LHError = require('../../lib/lh-error');

class LayoutStability extends MetricArtifact {

This comment was marked as resolved.

title: 'Cumulative Layout Shift',
// TODO(paulirish): improve this description.
/** Description of the Cumulative Layout Shift metric that indicates how much the page changes its layout while it loads. If big segments of the page shift their location during load, the Cumulative Layout Shift will be higher. This description is displayed within a tooltip when the user hovers on the metric name to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'The more the page\'s layout changes during its load, the higher the ' +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern we use is " ." So I took a stab at it.

"Cumulative Layout Shift is the sum of all layout shifts that occurred during a page's load. A layout shift is any movement an element makes once it is visible to the user. All layout shift is recorded, scored, and then aggregated into a cumulative score between 0 and 1; 0 being a perfectly stable page, and >=0.5 being a highly shifting page."


const finalLayoutShiftTraceEventFound = !!finalLayoutShift;
// tdresser sez: In about 10% of cases, layout instability is 0, and there will be no trace events.
// TODO: Validate that. http://crbug.com/1003459
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bug was fixed, is this still valid?

Suggested change
// TODO: Validate that. http://crbug.com/1003459
// TODO(http://crbug.com/1003459): Validate that.

@@ -376,6 +381,7 @@ const ERRORS = {
},

// Hey! When adding a new error type, update lighthouse-result.proto too.
// Only necessary for runtime errors, which come from artifacts or pageLoadErrors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this error will never be a runtime error, correct? Or it won't while it is a silent audit and not part of performance?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wrote this comment idk how it got here in pauls pr :P

Copy link
Collaborator

@connorjclark connorjclark Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"this" error? This comment is a continuation of the previous line, and is referring to when proto must be edited. All of these errors are runtime errors.

types/audit.d.ts Outdated
@@ -95,7 +95,7 @@ declare global {
/** A numeric value that has a meaning specific to the audit, e.g. the number of nodes in the DOM or the timestamp of a specific load event. More information can be found in the audit details, if present. */
numericValue: number;
/** The unit of `numericValue`, used when the consumer wishes to convert numericValue to a display string. A superset of https://tc39.es/proposal-unified-intl-numberformat/section6/locales-currencies-tz_proposed_out.html#sec-issanctionedsimpleunitidentifier */
numericUnit: 'byte'|'millisecond'|'element';
numericUnit: 'byte'|'millisecond'|'element'|'shiftarinos';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unitless

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants