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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(config): switch to throttling settings object #4879

Merged
merged 6 commits into from
Mar 29, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -137,7 +137,7 @@ class UnusedBytes extends Audit {
static createAuditResult(result, graph) {
const simulatorOptions = PredictivePerf.computeRTTAndServerResponseTime(graph);
// TODO: calibrate multipliers, see https://github.com/GoogleChrome/lighthouse/issues/820
Object.assign(simulatorOptions, {cpuTaskMultiplier: 1, layoutTaskMultiplier: 1});
Object.assign(simulatorOptions, {cpuSlowdownMultiplier: 1, layoutTaskMultiplier: 1});
const simulator = new LoadSimulator(graph, simulatorOptions);

const debugString = result.debugString;
Expand Down
8 changes: 8 additions & 0 deletions lighthouse-core/config/default.js
Expand Up @@ -6,12 +6,20 @@
'use strict';

const Driver = require('../gather/driver');
const emulation = require('../lib/emulation').settings;

/* eslint-disable max-len */

module.exports = {
settings: {
maxWaitForLoad: Driver.MAX_WAIT_FOR_FULLY_LOADED,
throttlingMethod: 'devtools',
throttling: {
requestLatency: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.latency,
downloadThroughput: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.downloadThroughput * 8,
Copy link
Member

Choose a reason for hiding this comment

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

this bits vs bytes thing is unfortunate to keep track of. Should we put the units in the property name? Or any other ideas how to simplify/keep it clear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding the units in the name isn't the worst if we keep the units reasonable, essentially everything throughput should be in bits until we have to translate to DevTools :)

requestLatencyMs
downloadThroughputKbps
uploadThroughputKbps

uploadThroughput: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.uploadThroughput * 8,
cpuSlowdownMultiplier: emulation.CPU_THROTTLE_METRICS.rate,
},
},
passes: [{
passName: 'defaultPass',
Expand Down
17 changes: 9 additions & 8 deletions lighthouse-core/gather/driver.js
Expand Up @@ -1033,13 +1033,15 @@ class Driver {
* @return {Promise<void>}
*/
async setThrottling(settings, passConfig) {
const throttleCpu = passConfig.useThrottling && !settings.disableCpuThrottling;
const throttleNetwork = passConfig.useThrottling && !settings.disableNetworkThrottling;
const cpuPromise = throttleCpu ?
emulation.enableCPUThrottling(this) :
if (settings.throttlingMethod !== 'devtools') {
Copy link
Member

Choose a reason for hiding this comment

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

we use setThrottling for goOnline and goOffline, so does this break that for non devtools throttling? We may need to separate those from this anyways as passConfig.useThrottling probably shouldn't affect a call to driver.goOffline() anyways.

We can kick that to another PR if it gets involved, but is there another quick fix in the meantime?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah good catch, we can go offline but never go back online atm if devtools throttling is off or useThrottling is false :), I'll clean this up it's a pretty easy fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done and tests added!

return;
}

const cpuPromise = passConfig.useThrottling ?
emulation.enableCPUThrottling(this, settings.throttling) :
emulation.disableCPUThrottling(this);
const networkPromise = throttleNetwork ?
emulation.enableNetworkThrottling(this) :
const networkPromise = passConfig.useThrottling ?
emulation.enableNetworkThrottling(this, settings.throttling) :
emulation.disableNetworkThrottling(this);

await Promise.all([cpuPromise, networkPromise]);
Expand All @@ -1056,8 +1058,7 @@ class Driver {
}

/**
* Enable internet connection, using emulated mobile settings if
* `options.settings.disableNetworkThrottling` is false.
* Enable internet connection, using emulated mobile settings if applicable.
* @param {{settings: LH.ConfigSettings, passConfig: LH.ConfigPass}} options
* @return {Promise<void>}
*/
Expand Down
12 changes: 6 additions & 6 deletions lighthouse-core/lib/dependency-graph/simulator/simulator.js
Expand Up @@ -22,7 +22,7 @@ const DEFAULT_THROUGHPUT = emulation.TYPICAL_MOBILE_THROTTLING_METRICS.targetDow
// same multiplier as Lighthouse uses for CPU emulation
const DEFAULT_CPU_TASK_MULTIPLIER = emulation.CPU_THROTTLE_METRICS.rate;
// layout tasks tend to be less CPU-bound and do not experience the same increase in duration
const DEFAULT_LAYOUT_TASK_MULTIPLIER = DEFAULT_CPU_TASK_MULTIPLIER / 2;
const DEFAULT_LAYOUT_TASK_MULTIPLIER = .5;
Copy link
Member

Choose a reason for hiding this comment

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

didn't we agree earlier we prefer a leading 0? 0.5

VERY BIG DEAL. not a nit whatsoever. 馃槧

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

haha sure done

// if a task takes more than 10 seconds it's usually a sign it isn't actually CPU bound and we're overestimating
const DEFAULT_MAXIMUM_CPU_TASK_DURATION = 10000;

Expand All @@ -45,7 +45,7 @@ class Simulator {
rtt: DEFAULT_RTT,
throughput: DEFAULT_THROUGHPUT,
maximumConcurrentRequests: DEFAULT_MAXIMUM_CONCURRENT_REQUESTS,
cpuTaskMultiplier: DEFAULT_CPU_TASK_MULTIPLIER,
cpuSlowdownMultiplier: DEFAULT_CPU_TASK_MULTIPLIER,
layoutTaskMultiplier: DEFAULT_LAYOUT_TASK_MULTIPLIER,
},
options
Expand All @@ -57,8 +57,8 @@ class Simulator {
TcpConnection.maximumSaturatedConnections(this._rtt, this._throughput),
this._options.maximumConcurrentRequests
);
this._cpuTaskMultiplier = this._options.cpuTaskMultiplier;
this._layoutTaskMultiplier = this._options.layoutTaskMultiplier;
this._cpuSlowdownMultiplier = this._options.cpuSlowdownMultiplier;
this._layoutTaskMultiplier = this._cpuSlowdownMultiplier * this._options.layoutTaskMultiplier;

this._nodeTiming = new Map();
this._numberInProgressByType = new Map();
Expand Down Expand Up @@ -206,7 +206,7 @@ class Simulator {
const timingData = this._nodeTiming.get(node);
const multiplier = (/** @type {CpuNode} */ (node)).didPerformLayout()
? this._layoutTaskMultiplier
: this._cpuTaskMultiplier;
: this._cpuSlowdownMultiplier;
const totalDuration = Math.min(
Math.round((/** @type {CpuNode} */ (node)).event.dur / 1000 * multiplier),
DEFAULT_MAXIMUM_CPU_TASK_DURATION
Expand Down Expand Up @@ -360,7 +360,7 @@ module.exports = Simulator;
* @property {number} [throughput]
* @property {number} [fallbackTTFB]
* @property {number} [maximumConcurrentRequests]
* @property {number} [cpuTaskMultiplier]
* @property {number} [cpuSlowdownMultiplier]
* @property {number} [layoutTaskMultiplier]
*/

77 changes: 66 additions & 11 deletions lighthouse-core/lib/emulation.js
Expand Up @@ -6,6 +6,8 @@
// @ts-nocheck
Copy link
Member

Choose a reason for hiding this comment

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

馃帀

'use strict';

const NBSP = '\xa0';

/**
* Nexus 5X metrics adapted from emulated_devices/module.json
*/
Expand Down Expand Up @@ -118,8 +120,23 @@ function enableNexus5X(driver) {
]);
}

function enableNetworkThrottling(driver) {
return driver.sendCommand('Network.emulateNetworkConditions', TYPICAL_MOBILE_THROTTLING_METRICS);
/**
* @param {Driver} driver
Copy link
Member

Choose a reason for hiding this comment

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

for type checking in this file we'll have to require Driver just to get the type (with an eslint ignore unused above it). Any thoughts on that style? It's pretty ugly but should have no real impact on a node run or browserify (and we obviously use driver so it won't ever be tree shaken if we switch bundling tools).

OTOH there's exposing the major types on LH so it could be {LH.Driver}, but that has its own ugliness (it's a real JS class, not an interface like the rest of our typings) and I have no idea what the impact on tsc performance will be as we continue to expand the global type graph :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah sg, I've used that before 馃憤

* @param {LH.ThrottlingSettings|undefined} throttlingSettings
* @return {Promise<void>}
*/
function enableNetworkThrottling(driver, throttlingSettings) {
let conditions = TYPICAL_MOBILE_THROTTLING_METRICS;
if (throttlingSettings) {
conditions = {
offline: false,
latency: throttlingSettings.requestLatency,
downloadThroughput: throttlingSettings.downloadThroughput / 8,
uploadThroughput: throttlingSettings.uploadThroughput / 8,
};
}

return driver.sendCommand('Network.emulateNetworkConditions', conditions);
}

function disableNetworkThrottling(driver) {
Expand All @@ -130,22 +147,60 @@ function goOffline(driver) {
return driver.sendCommand('Network.emulateNetworkConditions', OFFLINE_METRICS);
}

function enableCPUThrottling(driver) {
return driver.sendCommand('Emulation.setCPUThrottlingRate', CPU_THROTTLE_METRICS);
/**
* @param {Driver} driver
* @param {LH.ThrottlingSettings|undefined} throttlingSettings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brendankenny for some reason type checking was refusing to allow undefined as an input to LH.ThrottlingSettings=, no clue why, any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

looks like this is a temporary bug in the version of TS we're using. I was actually pleased that it started completely differentiating between optional and |undefined, but looks like that wasn't on purpose :) (there's a separate discussion around using void or some other kind of missing type to indicate the "optional but never undefined" case: microsoft/TypeScript#13195).

Later releases have that fixed, though. I'm about to make a PR bumping us to typescript@2.9.0-dev.20180323 (for reasons I'll explain there). You could get a jump on that.

* @return {Promise<void>}
*/
function enableCPUThrottling(driver, throttlingSettings) {
const rate = throttlingSettings
? throttlingSettings.cpuSlowdownMultiplier
: CPU_THROTTLE_METRICS.rate;
return driver.sendCommand('Emulation.setCPUThrottlingRate', {rate});
}

function disableCPUThrottling(driver) {
return driver.sendCommand('Emulation.setCPUThrottlingRate', NO_CPU_THROTTLE_METRICS);
}

function getEmulationDesc() {
const {latency, downloadThroughput, uploadThroughput} = TYPICAL_MOBILE_THROTTLING_METRICS;
const byteToMbit = bytes => (bytes / 1024 / 1024 * 8).toFixed(1);
/**
* @param {LH.ConfigSettings} settings
* @return {deviceEmulation: string, cpuThrottling: string, networkThrottling: string}
*/
function getEmulationDesc(settings) {
let cpuThrottling;
let networkThrottling;
const byteToKbit = bytes => (bytes / 1024 * 8).toFixed();

switch (settings.throttlingMethod) {
case 'provided':
cpuThrottling = 'Provided by environment';
networkThrottling = 'Provided by environment';
break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatency} = settings.throttling;
const down = byteToKbit(settings.throttling.downloadThroughput);
const up = byteToKbit(settings.throttling.uploadThroughput);

cpuThrottling = `${cpuSlowdownMultiplier}x slowdown (DevTools)`;
networkThrottling = `${requestLatency}${NBSP}ms HTTP RTT, ` +
`${down}${NBSP}Kbps down, ${up}${NBSP}Kbps up (DevTools)`;
break;
}
case 'simulate': {
const {cpuSlowdownMultiplier, rtt} = settings.throttling;
const throughput = byteToKbit(settings.throttling.throughput);
cpuThrottling = `${cpuSlowdownMultiplier}x slowdown (Simulated)`;
networkThrottling = `${rtt}${NBSP}ms TCP RTT, ` +
`${throughput}${NBSP}Kbps throughput (Simulated)`;
break;
}
}

return {
'deviceEmulation': 'Nexus 5X',
'cpuThrottling': `${CPU_THROTTLE_METRICS.rate}x slowdown`,
'networkThrottling': `${latency}ms RTT, ${byteToMbit(downloadThroughput)}Mbps down, ` +
`${byteToMbit(uploadThroughput)}Mbps up`,
deviceEmulation: settings.disableDeviceEmulation ? 'Disabled' : 'Nexus 5X',
Copy link
Member

Choose a reason for hiding this comment

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

馃憤 way better than the old enabled/disabled and a decription thing

cpuThrottling,
networkThrottling,
};
}

Expand Down
7 changes: 3 additions & 4 deletions lighthouse-core/lib/sentry.js
Expand Up @@ -82,12 +82,11 @@ sentryDelegate.init = function init(opts) {
);
}

const context = {
const context = Object.assign({
url: opts.url,
deviceEmulation: !opts.flags.disableDeviceEmulation,
networkThrottling: !opts.flags.disableNetworkThrottling,
cpuThrottling: !opts.flags.disableCpuThrottling,
};
throttlingMethod: opts.flags.throttlingMethod,
}, opts.flags.throttling);

sentryDelegate.mergeContext({extra: Object.assign({}, environmentData.extra, context)});
return context;
Expand Down
2 changes: 0 additions & 2 deletions lighthouse-core/report/v2/renderer/report-renderer.js
Expand Up @@ -69,8 +69,6 @@ class ReportRenderer {
const item = this._dom.cloneTemplate('#tmpl-lh-env__items', env);
this._dom.find('.lh-env__name', item).textContent = runtime.name;
this._dom.find('.lh-env__description', item).textContent = runtime.description;
this._dom.find('.lh-env__enabled', item).textContent =
runtime.enabled ? 'Enabled' : 'Disabled';
env.appendChild(item);
});

Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/report/v2/templates.html
Expand Up @@ -338,9 +338,8 @@ <h1 class="leftnav__header__title">Lighthouse</h1>
</li>
<template id="tmpl-lh-env__items">
<li class="lh-env__item">
<span class="lh-env__name"><!-- fill me --></span>
<span class="lh-env__description"><!-- fill me --></span>:
<b class="lh-env__enabled"><!-- fill me --></b>
<span class="lh-env__name"><!-- fill me --></span>:
<span class="lh-env__description"><!-- fill me --></span>
</li>
</template>
</ul>
Expand Down
5 changes: 1 addition & 4 deletions lighthouse-core/runner.js
Expand Up @@ -389,21 +389,18 @@ class Runner {
* @return {!Object} runtime config
*/
static getRuntimeConfig(settings) {
const emulationDesc = emulation.getEmulationDesc();
const emulationDesc = emulation.getEmulationDesc(settings);
const environment = [
{
name: 'Device Emulation',
enabled: !settings.disableDeviceEmulation,
description: emulationDesc['deviceEmulation'],
},
{
name: 'Network Throttling',
enabled: !settings.disableNetworkThrottling,
description: emulationDesc['networkThrottling'],
},
{
name: 'CPU Throttling',
enabled: !settings.disableCpuThrottling,
description: emulationDesc['cpuThrottling'],
},
];
Expand Down
36 changes: 22 additions & 14 deletions lighthouse-core/test/gather/gather-runner-test.js
Expand Up @@ -148,9 +148,9 @@ describe('GatherRunner', function() {
return GatherRunner.setupDriver(driver, {}, {
settings: {},
}).then(_ => {
assert.equal(tests.calledDeviceEmulation, true);
assert.equal(tests.calledNetworkEmulation, true);
assert.equal(tests.calledCpuEmulation, true);
assert.ok(tests.calledDeviceEmulation, 'did not call device emulation');
assert.ok(!tests.calledNetworkEmulation, 'called network emulation');
assert.ok(!tests.calledCpuEmulation, 'called cpu emulation');
});
});

Expand All @@ -173,6 +173,8 @@ describe('GatherRunner', function() {
return GatherRunner.setupDriver(driver, {}, {
settings: {
disableDeviceEmulation: true,
throttlingMethod: 'devtools',
throttling: {},
},
}).then(_ => {
assert.equal(tests.calledDeviceEmulation, false);
Expand All @@ -181,7 +183,7 @@ describe('GatherRunner', function() {
});
});

it('stops network throttling when disableNetworkThrottling flag is true', () => {
it('stops throttling when not devtools', () => {
const tests = {
calledDeviceEmulation: false,
calledNetworkEmulation: false,
Expand All @@ -199,18 +201,16 @@ describe('GatherRunner', function() {

return GatherRunner.setupDriver(driver, {}, {
settings: {
disableNetworkThrottling: true,
throttlingMethod: 'provided',
},
}).then(_ => {
assert.ok(tests.calledDeviceEmulation, 'called device emulation');
assert.deepEqual(tests.calledNetworkEmulation, [{
latency: 0, downloadThroughput: 0, uploadThroughput: 0, offline: false,
}]);
assert.ok(tests.calledCpuEmulation, 'called CPU emulation');
assert.ok(tests.calledDeviceEmulation, 'did not call device emulation');
assert.ok(!tests.calledNetworkEmulation, 'called network emulation');
assert.ok(!tests.calledCpuEmulation, 'called CPU emulation');
});
});

it('stops cpu throttling when disableCpuThrottling flag is true', () => {
it('sets throttling according to settings', () => {
const tests = {
calledDeviceEmulation: false,
calledNetworkEmulation: false,
Expand All @@ -228,11 +228,19 @@ describe('GatherRunner', function() {

return GatherRunner.setupDriver(driver, {}, {
settings: {
disableCpuThrottling: true,
throttlingMethod: 'devtools',
throttling: {
requestLatency: 100,
downloadThroughput: 800,
uploadThroughput: 800,
cpuSlowdownMultiplier: 1,
},
},
}).then(_ => {
assert.ok(tests.calledDeviceEmulation, 'called device emulation');
assert.ok(tests.calledNetworkEmulation, 'called network emulation');
assert.ok(tests.calledDeviceEmulation, 'did not call device emulation');
assert.deepEqual(tests.calledNetworkEmulation, [{
latency: 100, downloadThroughput: 100, uploadThroughput: 100, offline: false,
}]);
assert.deepEqual(tests.calledCpuEmulation, [{rate: 1}]);
});
});
Expand Down
Expand Up @@ -61,7 +61,10 @@ describe('DependencyGraph/Simulator', () => {
const cpuNode = new CpuNode(cpuTask({duration: 200}));
cpuNode.addDependency(rootNode);

const simulator = new Simulator(rootNode, {serverResponseTimeByOrigin, cpuTaskMultiplier: 5});
const simulator = new Simulator(rootNode, {
serverResponseTimeByOrigin,
cpuSlowdownMultiplier: 5,
});
const result = simulator.simulate();
// should be 2 RTTs and 500ms for the server response time + 200 CPU
assert.equal(result.timeInMs, 300 + 500 + 200);
Expand Down Expand Up @@ -99,7 +102,10 @@ describe('DependencyGraph/Simulator', () => {
nodeA.addDependent(nodeC);
nodeA.addDependent(nodeD);

const simulator = new Simulator(nodeA, {serverResponseTimeByOrigin, cpuTaskMultiplier: 5});
const simulator = new Simulator(nodeA, {
serverResponseTimeByOrigin,
cpuSlowdownMultiplier: 5,
});
const result = simulator.simulate();
// should be 800ms A, then 1000 ms total for B, C, D in serial
assert.equal(result.timeInMs, 1800);
Expand All @@ -123,7 +129,10 @@ describe('DependencyGraph/Simulator', () => {
nodeC.addDependent(nodeD);
nodeC.addDependent(nodeF); // finishes 400 ms after D

const simulator = new Simulator(nodeA, {serverResponseTimeByOrigin, cpuTaskMultiplier: 5});
const simulator = new Simulator(nodeA, {
serverResponseTimeByOrigin,
cpuSlowdownMultiplier: 5,
});
const result = simulator.simulate();
// should be 800ms each for A, B, C, D, with F finishing 400 ms after D
assert.equal(result.timeInMs, 3600);
Expand Down
Expand Up @@ -105,11 +105,9 @@ describe('ReportRenderer V2', () => {
assert.equal(userAgent.textContent, sampleResults.userAgent, 'user agent populated');

// Check runtime settings were populated.
const enables = header.querySelectorAll('.lh-env__enabled');
const names = Array.from(header.querySelectorAll('.lh-env__name')).slice(1);
const descriptions = header.querySelectorAll('.lh-env__description');
sampleResults.runtimeConfig.environment.forEach((env, i) => {
assert.equal(enables[i].textContent, env.enabled ? 'Enabled' : 'Disabled');
assert.equal(names[i].textContent, env.name);
assert.equal(descriptions[i].textContent, env.description);
});
Expand Down