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

patrickhulce · 2018-03-27T23:58:17Z

ok followup to ongoing work for config settings, I had to draw the line somewhere to keep this reasonable size, so in this PR it simply removes the two flags and changes our existing logic to use the throttling object instead.

upcoming PRs will have the 'simulate' throttlingMethod to actually return simulated metrics, but already at this stage you can do awesome stuff like lighthouse --throttling.requestLatency 2000 😃

I started to do the other constants cleanup here, but it started to balloon the PR

closes #4871

patrickhulce · 2018-03-27T23:59:47Z

note this is also blocking lots of pending PRs so timely 🕙reviews please! don't want to forced to start stuffing my followup work turning this into a mega PR 😆

patrickhulce · 2018-03-28T00:08:00Z

lighthouse-core/lib/emulation.js

-  return driver.sendCommand('Emulation.setCPUThrottlingRate', CPU_THROTTLE_METRICS);
+/**
+ * @param {Driver} driver
+ * @param {LH.ThrottlingSettings|undefined} throttlingSettings


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

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.

wardpeet

Nothing to add 🙄, not really helping but at least you got a review out of me 😄

Did we change base throttling as travis is failing?

patrickhulce · 2018-03-28T17:26:23Z

Did we change base throttling as travis is failing?

whoops good catch, didn't push my latest config tweak :D

brendankenny

Still reviewing, but some initial comments. I think this is going to be a real good change

brendankenny · 2018-03-28T18:41:03Z

lighthouse-core/config/default.js

+    throttlingMethod: 'devtools',
+    throttling: {
+      requestLatency: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.latency,
+      downloadThroughput: emulation.TYPICAL_MOBILE_THROTTLING_METRICS.downloadThroughput * 8,


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?

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

brendankenny · 2018-03-28T18:47:28Z

lighthouse-core/gather/driver.js

-    const throttleNetwork = passConfig.useThrottling && !settings.disableNetworkThrottling;
-    const cpuPromise = throttleCpu ?
-        emulation.enableCPUThrottling(this) :
+    if (settings.throttlingMethod !== 'devtools') {


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?

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

done and tests added!

brendankenny · 2018-03-28T19:04:36Z

lighthouse-core/lib/emulation.js

-  return driver.sendCommand('Emulation.setCPUThrottlingRate', CPU_THROTTLE_METRICS);
+/**
+ * @param {Driver} driver
+ * @param {LH.ThrottlingSettings|undefined} throttlingSettings


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.

brendankenny · 2018-03-28T20:37:45Z

lighthouse-core/lib/emulation.js

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


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 :)

yeah sg, I've used that before 👍

brendankenny

Except for the one Kbps question, LGTM. Great cleanup :)

brendankenny · 2018-03-29T00:34:43Z

lighthouse-core/lib/emulation.js

@@ -3,9 +3,12 @@
 * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
 * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
 */
-// @ts-nocheck


brendankenny · 2018-03-29T00:35:12Z

lighthouse-core/lib/emulation.js

-  // Injecting this function clientside is no longer neccessary as of m62. This is done
-  // on the backend when `Emulation.setTouchEmulationEnabled` is set.
-  //   https://bugs.chromium.org/p/chromium/issues/detail?id=133915#c63
-  // Once m62 hits stable (~Oct 20th) we can nuke this entirely


brendankenny · 2018-03-29T00:43:18Z

lighthouse-core/lib/emulation.js

+      break;
+    case 'devtools': {
+      const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
+      const down = byteToKbit(throttling.downloadThroughputKbps);


wait, shouldn't these already be Kbps?

whoops yes good catch forgot to update :)

brendankenny · 2018-03-29T00:51:09Z

lighthouse-core/lib/emulation.js

-    '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',


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

paulirish · 2018-03-29T20:21:18Z

lighthouse-core/lib/dependency-graph/simulator/simulator.js


 // 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;


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

VERY BIG DEAL. not a nit whatsoever. 😠

haha sure done

paulirish · 2018-03-29T20:24:22Z

lighthouse-core/lib/emulation.js

+      downloadThroughput: throttlingSettings.downloadThroughputKbps || 0,
+      uploadThroughput: throttlingSettings.uploadThroughputKbps || 0,
+    };
+  } else {


what case is this handling? if throttlingSettings are provided via config now, what is this fallback case for?

for configs that don't expressly set throttling or extend the default, also tests :)

this could be one of the things we (eventually) pull into config.js so once it comes out throttling.settings is always defined

core(config): switch to throttling settings object

917d48d

patrickhulce requested review from brendankenny, paulirish and vinamratasingal-zz as code owners March 27, 2018 23:58

devtools-bot added the waiting4reviewer label Mar 28, 2018

patrickhulce commented Mar 28, 2018

View reviewed changes

wardpeet approved these changes Mar 28, 2018

View reviewed changes

fix defaults :D

9fb8863

brendankenny requested changes Mar 28, 2018

View reviewed changes

brendankenny reviewed Mar 28, 2018

View reviewed changes

patrickhulce added 2 commits March 28, 2018 14:24

brendan feedback

2aa8527

type check emulation

967edc2

brendankenny approved these changes Mar 29, 2018

View reviewed changes

paulirish reviewed Mar 29, 2018

View reviewed changes

paulirish removed the request for review from vinamratasingal-zz March 29, 2018 20:25

patrickhulce added 2 commits March 29, 2018 13:34

units are hard

d0ee1cb

floor

bbf65b0

paulirish approved these changes Mar 29, 2018

View reviewed changes

patrickhulce merged commit 467453d into master Mar 29, 2018

patrickhulce deleted the switch_to_throttling_method branch March 29, 2018 23:11

paulirish removed the waiting4reviewer label Apr 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

patrickhulce commented Mar 27, 2018

patrickhulce commented Mar 27, 2018

patrickhulce Mar 28, 2018 •

edited

brendankenny Mar 28, 2018

wardpeet left a comment •

edited

patrickhulce commented Mar 28, 2018

brendankenny left a comment

brendankenny Mar 28, 2018

patrickhulce Mar 28, 2018

brendankenny Mar 28, 2018

patrickhulce Mar 28, 2018 •

edited

patrickhulce Mar 28, 2018

brendankenny Mar 28, 2018

brendankenny Mar 28, 2018

patrickhulce Mar 28, 2018

brendankenny left a comment

brendankenny Mar 29, 2018

brendankenny Mar 29, 2018

brendankenny Mar 29, 2018

patrickhulce Mar 29, 2018

brendankenny Mar 29, 2018 •

edited

paulirish Mar 29, 2018

patrickhulce Mar 29, 2018

paulirish Mar 29, 2018

patrickhulce Mar 29, 2018

brendankenny Mar 29, 2018

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

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

Conversation

patrickhulce commented Mar 27, 2018

patrickhulce commented Mar 27, 2018

patrickhulce Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet left a comment • edited

Choose a reason for hiding this comment

patrickhulce commented Mar 28, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Mar 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 28, 2018 •

edited

wardpeet left a comment •

edited

patrickhulce Mar 28, 2018 •

edited

brendankenny Mar 29, 2018 •

edited