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

core(speedindex): scale coefficients according to throttling #7007

Merged
merged 2 commits into from
Jan 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions lighthouse-core/computed/metrics/lantern-metric.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,19 @@ class LanternMetricArtifact {
throw new Error('COEFFICIENTS unimplemented!');
}

/**
* 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.
*
* @param {number} rttMs
* @return {LH.Gatherer.Simulation.MetricCoefficients}
*/
static getScaledCoefficients(rttMs) { // eslint-disable-line no-unused-vars
return this.COEFFICIENTS;
}

/**
* @param {Node} dependencyGraph
* @param {LH.Artifacts.TraceOfTab} traceOfTab
Expand Down Expand Up @@ -107,13 +120,14 @@ class LanternMetricArtifact {
Object.assign({}, extras, {optimistic: false})
);

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 = 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,
Expand Down
28 changes: 28 additions & 0 deletions lighthouse-core/computed/metrics/lantern-speed-index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand All @@ -28,6 +29,33 @@ class LanternSpeedIndex extends LanternMetric {
};
}

/**
* @param {number} rttMs
* @return {LH.Gatherer.Simulation.MetricCoefficients}
*/
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);

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}
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/lib/dependency-graph/simulator/simulator.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,11 @@ class Simulator {
this._connectionPool = /** @type {ConnectionPool} */ (null);
}

/** @return {number} */
get rtt() {
return this._rtt;
}

/**
* @param {Node} graph
*/
Expand Down

This file was deleted.

50 changes: 48 additions & 2 deletions lighthouse-core/test/computed/metrics/lantern-speed-index-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -13,14 +14,59 @@ 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);

expect({
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,
}
`);
Copy link
Member

Choose a reason for hiding this comment

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

lol

});

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.getScaledCoefficients(defaultThrottling.rttMs);
expect(result).toEqual(LanternSpeedIndex.COEFFICIENTS);
});

it('should scale coefficients back', async () => {
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.getScaledCoefficients(300);
expect(result).toMatchInlineSnapshot(`
Object {
"intercept": -562.5,
"optimistic": 2.525,
"pessimistic": 0.8375,
}
`);
});
});