From 5aec9c74118711dedbd4720e16ca5b61c9bd6407 Mon Sep 17 00:00:00 2001 From: Deepanjan Roy Date: Tue, 28 May 2019 10:10:08 -0400 Subject: [PATCH] Address all review comments --- .../metrics/cumulative-long-queuing-delay.js | 32 +++++++++---------- .../metrics/cumulative-long-queuing-delay.js | 7 ++++ lighthouse-core/lib/i18n/en-US.json | 8 ----- .../cumulative-long-queuing-delay-test.js | 11 +++---- .../cumulative-long-queuing-delay-test.js | 21 ++++++++++-- lighthouse-core/test/results/sample_v2.json | 16 ++-------- proto/sample_v2_round_trip.json | 4 +-- 7 files changed, 50 insertions(+), 49 deletions(-) diff --git a/lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js b/lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js index dd1983f3f14a..002f2df3124c 100644 --- a/lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js +++ b/lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js @@ -6,17 +6,15 @@ 'use strict'; const Audit = require('../audit'); -const i18n = require('../../lib/i18n/i18n.js'); const CumulativeLQD = require('../../computed/metrics/cumulative-long-queuing-delay.js'); -const UIStrings = { +// TODO(deepanjanroy): i18n strings once metric is final. +const UIStringsNotExported = { title: 'Cumulative Long Queuing Delay', - description: '[Experimental metric]. Sum of Task Lengths beyond 50ms, between ' + - 'First Contentful Paint and Time To Interactive.', + description: '[Experimental metric] Total time period between FCP and Time to Interactive ' + + 'during which queuing time for any input event would be higher than 50ms.', }; -const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); - class CumulativeLongQueuingDelay extends Audit { /** * @return {LH.Audit.Meta} @@ -24,8 +22,8 @@ class CumulativeLongQueuingDelay extends Audit { static get meta() { return { id: 'cumulative-long-queuing-delay', - title: str_(UIStrings.title), - description: str_(UIStrings.description), + title: UIStringsNotExported.title, + description: UIStringsNotExported.description, scoreDisplayMode: Audit.SCORING_MODES.NUMERIC, requiredArtifacts: ['traces', 'devtoolsLogs'], }; @@ -36,13 +34,14 @@ class CumulativeLongQueuingDelay extends Audit { */ static get defaultOptions() { return { - // Generally, the scoreMedian and scorePODR value is set to be real world 25th/75th and - // 5th/95th percentile value respectively. According to a cluster telemetry run over top 10k - // sites on mobile, 25-th percentile was 270ms, 10-th percentile was 22ms, and 5th percentile - // was 0ms. These numbers include 404 pages, so rounding up the scoreMedian to 300ms and - // picking 25ms as PODR. See curve at https://www.desmos.com/calculator/x3nzenjyln - scoreMedian: 300, - scorePODR: 25, + // According to a cluster telemetry run over top 10k sites on mobile, 5th percentile was 0ms, + // 25th percentile was 270ms and median was 895ms. These numbers include 404 pages. Picking + // thresholds according to our 25/75-th rule will be quite harsh scoring (a single 350ms task) + // after FCP will yield a score of .5. The following coefficients are semi-arbitrarily picked + // to give 600ms jank a score of .5 and 100ms jank a score of .999. We can tweak these numbers + // in the future. See https://www.desmos.com/calculator/a7ib75kq3g + scoreMedian: 600, + scorePODR: 200, }; } @@ -72,10 +71,9 @@ class CumulativeLongQueuingDelay extends Audit { context.options.scoreMedian ), numericValue: metricResult.timing, - displayValue: str_(i18n.UIStrings.ms, {timeInMs: metricResult.timing}), + displayValue: 10 * Math.round(metricResult.timing / 10) + '\xa0ms', }; } } module.exports = CumulativeLongQueuingDelay; -module.exports.UIStrings = UIStrings; diff --git a/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js b/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js index f41acb9ca89b..965355a27c14 100644 --- a/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js +++ b/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js @@ -20,6 +20,11 @@ const TimetoInteractive = require('./interactive.js'); * task, the first 60ms of it is Long Queuing Delay Region, because any input event occuring in * that region has to wait more than 50ms. Cumulative Long Queuing Delay is the sum of all Long * Queuing Delay Regions between First Contentful Paint and Interactive Time (TTI). + * + * This is a new metric designed to accompany Time to Interactive. TTI is strict and does not + * reflect incremental improvements to the site performance unless the improvement concerns the last + * long task. Cumulative Long Queuing Delay on the other hand is designed to be much more responsive + * to smaller improvements to main thread responsiveness. */ class CumulativeLongQueuingDelay extends ComputedMetric { /** @@ -35,6 +40,8 @@ class CumulativeLongQueuingDelay extends ComputedMetric { * @return {number} */ static calculateSumOfLongQueuingDelay(topLevelEvents, fcpTimeInMs, interactiveTimeMs) { + if (interactiveTimeMs <= fcpTimeInMs) return 0; + const threshold = CumulativeLongQueuingDelay.LONG_QUEUING_DELAY_THRESHOLD; const longQueuingDelayRegions = []; // First identifying the long queuing delay regions. diff --git a/lighthouse-core/lib/i18n/en-US.json b/lighthouse-core/lib/i18n/en-US.json index 306ca993e27a..02e444d58783 100644 --- a/lighthouse-core/lib/i18n/en-US.json +++ b/lighthouse-core/lib/i18n/en-US.json @@ -671,14 +671,6 @@ "message": "Minimizes main-thread work", "description": "Title of a diagnostic audit that provides detail on the main thread work the browser did to load the page. This descriptive title is shown to users when the amount is acceptable and no user action is required." }, - "lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js | description": { - "message": "[Experimental metric]. Sum of Task Lengths beyond 50ms, between First Contentful Paint and Time To Interactive.", - "description": "" - }, - "lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js | title": { - "message": "Cumulative Long Queuing Delay", - "description": "" - }, "lighthouse-core/audits/metrics/estimated-input-latency.js | description": { "message": "Estimated Input Latency is an estimate of how long your app takes to respond to user input, in milliseconds, during the busiest 5s window of page load. If your latency is higher than 50 ms, users may perceive your app as laggy. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/estimated-input-latency).", "description": "Description of the Estimated Input Latency metric that estimates the amount of time, in milliseconds, that the app takes to respond to user input. 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." diff --git a/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js b/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js index d011b48e0599..641f64796f08 100644 --- a/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js +++ b/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js @@ -19,15 +19,14 @@ function generateArtifactsWithTrace(trace) { /* eslint-env jest */ describe('Performance: cumulative-long-queuing-delay audit', () => { - it('evaluates cumulative long queuing delay metric properly', () => { + it('evaluates cumulative long queuing delay metric properly', async () => { const artifacts = generateArtifactsWithTrace(pwaTrace); const settings = {throttlingMethod: 'provided'}; const context = {options, settings, computedCache: new Map()}; + const output = await Audit.audit(artifacts, context); - return Audit.audit(artifacts, context).then(output => { - expect(output.numericValue).toBeCloseTo(48.3, 1); - expect(output.score).toBeCloseTo(0.97, 2); - expect(output.displayValue).toBeDisplayString('50\xa0ms'); - }); + expect(output.numericValue).toBeCloseTo(48.3, 1); + expect(output.score).toBeCloseTo(1, 2); + expect(output.displayValue).toBeDisplayString('50\xa0ms'); }); }); diff --git a/lighthouse-core/test/computed/metrics/cumulative-long-queuing-delay-test.js b/lighthouse-core/test/computed/metrics/cumulative-long-queuing-delay-test.js index ea9cbdf9a5d2..8b02ada00c13 100644 --- a/lighthouse-core/test/computed/metrics/cumulative-long-queuing-delay-test.js +++ b/lighthouse-core/test/computed/metrics/cumulative-long-queuing-delay-test.js @@ -45,7 +45,7 @@ Object { }); describe('#calculateSumOfLongQueuingDelay', () => { - it('reports 0 when no task is longer than 50ms', async () => { + it('reports 0 when no task is longer than 50ms', () => { const events = [ {start: 1000, end: 1050, duration: 50}, {start: 2000, end: 2010, duration: 10}, @@ -85,7 +85,7 @@ Object { const events = [ {start: 1000, end: 1110, duration: 110}, // Contributes 10ms. - {start: 2000, end: 2100, duration: 100}, // Contributes 50ms. + {start: 2000, end: 2200, duration: 200}, // Contributes 50ms. ]; expect(CumulativeLongQueuingDelay.calculateSumOfLongQueuingDelay( @@ -94,5 +94,22 @@ Object { interactiveTimeMs )).toBe(60); }); + + // This can happen in the lantern metric case, where we use the optimistic + // TTI and pessimistic FCP. + it('returns 0 if interactiveTime is earlier than FCP', () => { + const fcpTimeMs = 2050; + const interactiveTimeMs = 1050; + + const events = [ + {start: 500, end: 3000, duration: 2500}, + ]; + + expect(CumulativeLongQueuingDelay.calculateSumOfLongQueuingDelay( + events, + fcpTimeMs, + interactiveTimeMs + )).toBe(0); + }); }); }); diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index dcdf66e802d3..00d57e2385a8 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -194,8 +194,8 @@ "cumulative-long-queuing-delay": { "id": "cumulative-long-queuing-delay", "title": "Cumulative Long Queuing Delay", - "description": "[Experimental metric]. Sum of Task Lengths beyond 50ms, between First Contentful Paint and Time To Interactive.", - "score": 0.83, + "description": "[Experimental metric] Total time period between FCP and Time to Interactive during which queuing time for any input event would be higher than 50ms.", + "score": 1, "scoreDisplayMode": "numeric", "numericValue": 116.79800000000023, "displayValue": "120 ms" @@ -5105,12 +5105,6 @@ }, "path": "audits[estimated-input-latency].displayValue" }, - { - "values": { - "timeInMs": 116.79800000000023 - }, - "path": "audits[cumulative-long-queuing-delay].displayValue" - }, { "values": { "timeInMs": 122.537 @@ -5130,12 +5124,6 @@ "path": "audits[network-server-latency].displayValue" } ], - "lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js | title": [ - "audits[cumulative-long-queuing-delay].title" - ], - "lighthouse-core/audits/metrics/cumulative-long-queuing-delay.js | description": [ - "audits[cumulative-long-queuing-delay].description" - ], "lighthouse-core/audits/metrics/max-potential-fid.js | title": [ "audits[max-potential-fid].title" ], diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json index fee824bd2451..5a15c8194e29 100644 --- a/proto/sample_v2_round_trip.json +++ b/proto/sample_v2_round_trip.json @@ -368,11 +368,11 @@ "title": "Minimize Critical Requests Depth" }, "cumulative-long-queuing-delay": { - "description": "[Experimental metric]. Sum of Task Lengths beyond 50ms, between First Contentful Paint and Time To Interactive.", + "description": "[Experimental metric] Total time period between FCP and Time to Interactive during which queuing time for any input event would be higher than 50ms.", "displayValue": "120\u00a0ms", "id": "cumulative-long-queuing-delay", "numericValue": 116.79800000000023, - "score": 0.83, + "score": 1.0, "scoreDisplayMode": "numeric", "title": "Cumulative Long Queuing Delay" },