From e5ce2e35d29aa31d1d4805bab2bc352bd0699384 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 14 Jan 2019 15:10:56 -0600 Subject: [PATCH 1/2] core(speedindex): scale coefficients according to throttling --- .../computed/metrics/lantern-metric.js | 22 ++++++-- .../computed/metrics/lantern-speed-index.js | 20 ++++++++ .../dependency-graph/simulator/simulator.js | 5 ++ .../lantern-speed-index-test.js.snap | 9 ---- .../metrics/lantern-speed-index-test.js | 50 ++++++++++++++++++- 5 files changed, 91 insertions(+), 15 deletions(-) delete mode 100644 lighthouse-core/test/computed/metrics/__snapshots__/lantern-speed-index-test.js.snap diff --git a/lighthouse-core/computed/metrics/lantern-metric.js b/lighthouse-core/computed/metrics/lantern-metric.js index 9362d589b7ff..383f5a5add8c 100644 --- a/lighthouse-core/computed/metrics/lantern-metric.js +++ b/lighthouse-core/computed/metrics/lantern-metric.js @@ -42,6 +42,19 @@ class LanternMetricArtifact { throw new Error('COEFFICIENTS unimplemented!'); } + /** + * Scale the coefficients according to the throttling settings. + * Some lantern metrics (speed-index) use components in their estimate that are not + * from the simulator. In this case, we need to adjust the coefficients as the target throttling + * settings change. + * + * @param {number} rttMs + * @return {LH.Gatherer.Simulation.MetricCoefficients} + */ + static scaleCoefficients(rttMs) { // eslint-disable-line no-unused-vars + return this.COEFFICIENTS; + } + /** * @param {Node} dependencyGraph * @param {LH.Artifacts.TraceOfTab} traceOfTab @@ -107,13 +120,14 @@ class LanternMetricArtifact { Object.assign({}, extras, {optimistic: false}) ); + const coefficients = this.scaleCoefficients(simulator.rtt); // Estimates under 1s don't really follow the normal curve fit, minimize the impact of the intercept - const interceptMultiplier = this.COEFFICIENTS.intercept > 0 ? + const interceptMultiplier = coefficients.intercept > 0 ? Math.min(1, optimisticEstimate.timeInMs / 1000) : 1; const timing = - this.COEFFICIENTS.intercept * interceptMultiplier + - this.COEFFICIENTS.optimistic * optimisticEstimate.timeInMs + - this.COEFFICIENTS.pessimistic * pessimisticEstimate.timeInMs; + coefficients.intercept * interceptMultiplier + + coefficients.optimistic * optimisticEstimate.timeInMs + + coefficients.pessimistic * pessimisticEstimate.timeInMs; return { timing, diff --git a/lighthouse-core/computed/metrics/lantern-speed-index.js b/lighthouse-core/computed/metrics/lantern-speed-index.js index 7b6c5d53dafa..039767540961 100644 --- a/lighthouse-core/computed/metrics/lantern-speed-index.js +++ b/lighthouse-core/computed/metrics/lantern-speed-index.js @@ -10,6 +10,7 @@ const LanternMetric = require('./lantern-metric.js'); const BaseNode = require('../../lib/dependency-graph/base-node.js'); const Speedline = require('../speedline.js'); const LanternFirstContentfulPaint = require('./lantern-first-contentful-paint.js'); +const defaultThrottling = require('../../config/constants.js').throttling; /** @typedef {BaseNode.Node} Node */ @@ -28,6 +29,25 @@ class LanternSpeedIndex extends LanternMetric { }; } + /** + * @param {number} rttMs + * @return {LH.Gatherer.Simulation.MetricCoefficients} + */ + static scaleCoefficients(rttMs) { // eslint-disable-line no-unused-vars + // We want to multiply our default coefficients based on how much farther from baseline our + // new throttling settings are compared to the defaults. + // We will use a baseline of 30 ms RTT (where Speed Index should be a ~50/50 blend of optimistic/pessimistic). + const defaultCoefficients = this.COEFFICIENTS; + const defaultRttExcess = defaultThrottling.mobileSlow4G.rttMs - 30; + const multiplier = Math.max((rttMs - 30) / defaultRttExcess, 0); + + return { + intercept: defaultCoefficients.intercept * multiplier, + optimistic: 0.5 + (defaultCoefficients.optimistic - 0.5) * multiplier, + pessimistic: 0.5 + (defaultCoefficients.pessimistic - 0.5) * multiplier, + }; + } + /** * @param {Node} dependencyGraph * @return {Node} diff --git a/lighthouse-core/lib/dependency-graph/simulator/simulator.js b/lighthouse-core/lib/dependency-graph/simulator/simulator.js index d3b92b5dddaf..fe46d21c207e 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/simulator.js +++ b/lighthouse-core/lib/dependency-graph/simulator/simulator.js @@ -75,6 +75,11 @@ class Simulator { this._connectionPool = /** @type {ConnectionPool} */ (null); } + /** @return {number} */ + get rtt() { + return this._rtt; + } + /** * @param {Node} graph */ diff --git a/lighthouse-core/test/computed/metrics/__snapshots__/lantern-speed-index-test.js.snap b/lighthouse-core/test/computed/metrics/__snapshots__/lantern-speed-index-test.js.snap deleted file mode 100644 index 8854004ffa01..000000000000 --- a/lighthouse-core/test/computed/metrics/__snapshots__/lantern-speed-index-test.js.snap +++ /dev/null @@ -1,9 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`Metrics: Lantern Speed Index should compute predicted value 1`] = ` -Object { - "optimistic": 605, - "pessimistic": 1631, - "timing": 1657, -} -`; diff --git a/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js b/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js index f5cc91656895..92051dc47a27 100644 --- a/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js +++ b/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js @@ -5,6 +5,7 @@ */ 'use strict'; +const defaultThrottling = require('../../../config/constants.js').throttling.mobileSlow4G; const LanternSpeedIndex = require('../../../computed/metrics/lantern-speed-index.js'); const trace = require('../../fixtures/traces/progressive-app-m60.json'); @@ -13,7 +14,7 @@ const devtoolsLog = require('../../fixtures/traces/progressive-app-m60.devtools. /* eslint-env jest */ describe('Metrics: Lantern Speed Index', () => { it('should compute predicted value', async () => { - const settings = {}; + const settings = {throttlingMethod: 'simulate', throttling: defaultThrottling}; const context = {settings, computedCache: new Map()}; const result = await LanternSpeedIndex.request({trace, devtoolsLog, settings}, context); @@ -21,6 +22,51 @@ describe('Metrics: Lantern Speed Index', () => { timing: Math.round(result.timing), optimistic: Math.round(result.optimisticEstimate.timeInMs), pessimistic: Math.round(result.pessimisticEstimate.timeInMs), - }).toMatchSnapshot(); + }).toMatchInlineSnapshot(` +Object { + "optimistic": 605, + "pessimistic": 1631, + "timing": 1657, +} +`); + }); + + it('should compute predicted value for different settings', async () => { + const settings = {throttlingMethod: 'simulate', throttling: {...defaultThrottling, rttMs: 300}}; + const context = {settings, computedCache: new Map()}; + const result = await LanternSpeedIndex.request({trace, devtoolsLog, settings}, context); + + expect({ + timing: Math.round(result.timing), + optimistic: Math.round(result.optimisticEstimate.timeInMs), + pessimistic: Math.round(result.pessimisticEstimate.timeInMs), + }).toMatchInlineSnapshot(` +Object { + "optimistic": 605, + "pessimistic": 2381, + "timing": 2958, +} +`); + }); + + it('should not scale coefficients at default', async () => { + const result = LanternSpeedIndex.scaleCoefficients(defaultThrottling.rttMs); + expect(result).toEqual(LanternSpeedIndex.COEFFICIENTS); + }); + + it('should scale coefficients back', async () => { + const result = LanternSpeedIndex.scaleCoefficients(5); + expect(result).toEqual({intercept: -0, pessimistic: 0.5, optimistic: 0.5}); + }); + + it('should scale coefficients forward', async () => { + const result = LanternSpeedIndex.scaleCoefficients(300); + expect(result).toMatchInlineSnapshot(` +Object { + "intercept": -562.5, + "optimistic": 2.525, + "pessimistic": 0.8375, +} +`); }); }); From 523f5d883ed8995c2d09d6da007b8c6390fdfeac Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 14 Jan 2019 21:14:43 -0600 Subject: [PATCH 2/2] comments galore --- .../computed/metrics/lantern-metric.js | 6 +++--- .../computed/metrics/lantern-speed-index.js | 16 ++++++++++++---- .../computed/metrics/lantern-speed-index-test.js | 6 +++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lighthouse-core/computed/metrics/lantern-metric.js b/lighthouse-core/computed/metrics/lantern-metric.js index 383f5a5add8c..5c6fd1c502d7 100644 --- a/lighthouse-core/computed/metrics/lantern-metric.js +++ b/lighthouse-core/computed/metrics/lantern-metric.js @@ -43,7 +43,7 @@ class LanternMetricArtifact { } /** - * Scale the coefficients according to the throttling settings. + * Returns the coefficients, scaled by the throttling settings if needed by the metric. * Some lantern metrics (speed-index) use components in their estimate that are not * from the simulator. In this case, we need to adjust the coefficients as the target throttling * settings change. @@ -51,7 +51,7 @@ class LanternMetricArtifact { * @param {number} rttMs * @return {LH.Gatherer.Simulation.MetricCoefficients} */ - static scaleCoefficients(rttMs) { // eslint-disable-line no-unused-vars + static getScaledCoefficients(rttMs) { // eslint-disable-line no-unused-vars return this.COEFFICIENTS; } @@ -120,7 +120,7 @@ class LanternMetricArtifact { Object.assign({}, extras, {optimistic: false}) ); - const coefficients = this.scaleCoefficients(simulator.rtt); + const coefficients = this.getScaledCoefficients(simulator.rtt); // Estimates under 1s don't really follow the normal curve fit, minimize the impact of the intercept const interceptMultiplier = coefficients.intercept > 0 ? Math.min(1, optimisticEstimate.timeInMs / 1000) : 1; diff --git a/lighthouse-core/computed/metrics/lantern-speed-index.js b/lighthouse-core/computed/metrics/lantern-speed-index.js index 039767540961..5e38e168df17 100644 --- a/lighthouse-core/computed/metrics/lantern-speed-index.js +++ b/lighthouse-core/computed/metrics/lantern-speed-index.js @@ -33,10 +33,18 @@ class LanternSpeedIndex extends LanternMetric { * @param {number} rttMs * @return {LH.Gatherer.Simulation.MetricCoefficients} */ - static scaleCoefficients(rttMs) { // eslint-disable-line no-unused-vars - // We want to multiply our default coefficients based on how much farther from baseline our - // new throttling settings are compared to the defaults. - // We will use a baseline of 30 ms RTT (where Speed Index should be a ~50/50 blend of optimistic/pessimistic). + static getScaledCoefficients(rttMs) { // eslint-disable-line no-unused-vars + // We want to scale our default coefficients based on the speed of the connection. + // We will linearly interpolate coefficients for the passed-in rttMs based on two pre-determined points: + // 1. Baseline point of 30 ms RTT where Speed Index should be a ~50/50 blend of optimistic/pessimistic. + // 30 ms was based on a typical home WiFi connection's actual RTT. + // Coefficients here follow from the fact that the optimistic estimate should be very close + // to reality at this connection speed and the pessimistic estimate compensates for minor + // connection speed differences. + // 2. Default throttled point of 150 ms RTT where the default coefficients have been determined to be most accurate. + // Coefficients here were determined through thorough analysis and linear regression on the + // lantern test data set. See lighthouse-core/scripts/test-lantern.sh for more detail. + // While the coefficients haven't been analyzed at the interpolated points, it's our current best effort. const defaultCoefficients = this.COEFFICIENTS; const defaultRttExcess = defaultThrottling.mobileSlow4G.rttMs - 30; const multiplier = Math.max((rttMs - 30) / defaultRttExcess, 0); diff --git a/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js b/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js index 92051dc47a27..792a954ebcba 100644 --- a/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js +++ b/lighthouse-core/test/computed/metrics/lantern-speed-index-test.js @@ -50,17 +50,17 @@ Object { }); it('should not scale coefficients at default', async () => { - const result = LanternSpeedIndex.scaleCoefficients(defaultThrottling.rttMs); + const result = LanternSpeedIndex.getScaledCoefficients(defaultThrottling.rttMs); expect(result).toEqual(LanternSpeedIndex.COEFFICIENTS); }); it('should scale coefficients back', async () => { - const result = LanternSpeedIndex.scaleCoefficients(5); + const result = LanternSpeedIndex.getScaledCoefficients(5); expect(result).toEqual({intercept: -0, pessimistic: 0.5, optimistic: 0.5}); }); it('should scale coefficients forward', async () => { - const result = LanternSpeedIndex.scaleCoefficients(300); + const result = LanternSpeedIndex.getScaledCoefficients(300); expect(result).toMatchInlineSnapshot(` Object { "intercept": -562.5,