Add cpu throttle #747

wardpeet · 2016-10-05T05:03:00Z

No description provided.

wardpeet · 2016-10-05T05:03:19Z

is there any way I can check if throttling actually works?

paulirish · 2016-10-05T05:22:35Z

is there any way I can check if throttling actually works?

iterations = 0;
while (iterations < 1000){
   somethingExpensive();
   iterations++;
}

run that in a script tag in the head and our existing perf metrics should capture the cost.

addyosmani · 2016-10-05T17:07:04Z

lighthouse-core/lib/emulation.js

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


Just a thought: I'm curious if it's possible for us to get clever with Lighthouse's usage of CPU throttling. As throttling is relative to the system you're testing on, you can experience some pretty wildly different results. 5x throttling on a high-end Mac is going to be different to 5x throttling on a lower end computer. I wonder if it might be possible to do a pass that could determine system "highendness" and normalize throttling accordingly.

I see, we could use require('os') and check cpu ghz
require('os').cpus()

[ { model: 'Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz', speed: 3100, times: { user: 6248100, nice: 0, sys: 5165850, idle: 28768980, irq: 0 } }, { model: 'Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz', speed: 3100, times: { user: 2609450, nice: 0, sys: 1979810, idle: 35592290, irq: 0 } }, { model: 'Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz', speed: 3100, times: { user: 5899370, nice: 0, sys: 4368220, idle: 29913970, irq: 0 } }, { model: 'Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz', speed: 3100, times: { user: 2701570, nice: 0, sys: 2006030, idle: 35473940, irq: 0 } } ]

Prolly worth seeing what senor @paulirish thinks of the idea :)

Ayup, previous discussion over in #33.

We definitely want something browser friendly, so I don't think we should rely on advertised clock speed via the os module.

IMO calibrating the throttling rate so that all machines achieve an even result is totally worthwhile, but it's something we should start with a design doc and a framework for measurement. IMO the x5 multiplier will tide us over for a while, and we can pick up calibration in a quarter or so.

We could use https://developer.chrome.com/extensions/system_cpu in the extension and require('os') in the cli and update it later if we know a better solution

IMO the 5X will tide us over for a while, and we can pick up calibration in a quarter or so.

👍

FWIW an early patch for cpu throttling allowed the user to put in their octane results. We would use those to compare against saved results for mobile devices.

paulirish · 2016-10-05T21:50:09Z

lighthouse-core/gather/drivers/driver.js

@@ -492,7 +492,8 @@ class Driver {
  beginEmulation() {
    return Promise.all([
      emulation.enableNexus5X(this),
-      emulation.enableNetworkThrottling(this)
+      emulation.enableNetworkThrottling(this),


can you add the disable flags in this PR too?
#646 (comment)

while you're in there... :)

can you add the --disable-device-emulation flag as well? So you'll be adding all three: --disable-device-emulation --disable-cpu-throttling --disable-network-throttling

should I remove --mobile? or keep it as a shortcut?

paulirish · 2016-10-05T21:50:11Z

lighthouse-core/gather/drivers/driver.js

@@ -492,7 +492,8 @@ class Driver {
  beginEmulation() {
    return Promise.all([
      emulation.enableNexus5X(this),
-      emulation.enableNetworkThrottling(this)
+      emulation.enableNetworkThrottling(this),
+      //emulation.enableCPUThrottling(this),


CPU throttling should be on by default. Disabled if turned off by the flag.

If users are running LH against real phones, they'll have to do --disable-device-emulation --disable-cpu-throttling --disable-network-throttling themselves for now.

Worth this PR including a one-liner comment to the README that mentions that requirement for real phones?

Thanks @wardpeet!

@addyosmani is this good enough? https://github.com/GoogleChrome/lighthouse/pull/747/files#diff-0730bb7c2e8f9ea2438b52e419dd86c9R135

wardpeet · 2016-10-11T05:42:55Z

I have to update the tests so travis will fail but you can already have a look :)

paulirish · 2016-10-11T06:30:52Z

lighthouse-cli/index.js

@@ -61,6 +61,9 @@ const cli = yargs
  ], 'Configuration:')
  .describe({
    'mobile': 'Emulates a Nexus 5X',


let's nuke the mobile flag. sorry i wasn't explicit about that but that's what i meant to say in #646

paulirish · 2016-10-11T06:31:26Z

lighthouse-core/gather/drivers/driver.js

+  beginEmulation(flags) {
+    let emulations = [];
+
+    if (!flags.enableDeviceEmulation && flags.mobile) {


enable -> disable?

you coming back on these? would love the help. needed these flags yesterday. :)

yes fixing this now :p

wardpeet · 2016-10-14T06:41:43Z

@paulirish fixed and updated with tests
sadly I don't like the test cases, maybe we should add some kind of mock framework?

disable-device-emulation, disable-cpu-throttling, disable-network-throttling

brendankenny

review!

brendankenny · 2016-10-20T01:46:30Z

lighthouse-core/gather/drivers/driver.js

@@ -506,7 +517,7 @@ class Driver {

  /**
   * Enable internet connection, using emulated mobile settings if
-   * `options.flags.mobile` is true.
+   * `options.flags.disableNetworkThrottling` is false.


mobile flag is still used below

brendankenny · 2016-10-20T01:49:45Z

lighthouse-core/gather/drivers/driver.js

-      emulation.enableNetworkThrottling(this)
-    ]);
+  beginEmulation(flags) {
+    let emulations = [];


Reason why I don't use const here is because I modify the object. When using const I assume the data is not going to be changed. But I can change it :)

brendankenny · 2016-10-20T02:00:29Z

lighthouse-core/gather/gather-runner.js

-    // Enable emulation if required.
-    return Promise.resolve(options.flags.mobile && driver.beginEmulation())
+    // Enable emulation
+    // Will check what emulation needs to be done based on flags


one line comment, maybe // Enable emulation based on flags ?

brendankenny · 2016-10-20T02:00:32Z

lighthouse-core/gather/gather-runner.js

-    return Promise.resolve(options.flags.mobile && driver.beginEmulation())
+    // Enable emulation
+    // Will check what emulation needs to be done based on flags
+    return Promise.resolve(driver.beginEmulation(options.flags))


can drop the Promise.resolve, just return driver.beginEmulation(options.flags)

correct :) returns a promise anyway

brendankenny · 2016-10-20T02:02:28Z

readme.md

-  --list-trace-categories  Prints a list of all required trace categories and exits   [boolean]
-  --config-path            The path to the config JSON.
-  --perf                   Use a performance-test-only configuration                  [boolean]
+  --mobile                      Emulates a Nexus 5X                                  [default: true]


--mobile still here. This needs to be regenerated?

@wardpeet can you file an followup issue for our calibration-based throttling?

@paulirish see #820

brendankenny · 2016-10-20T02:03:06Z

readme.md

 ```

 ## Lighthouse w/ mobile devices

 Lighthouse can run against a real mobile device. You can follow the [Remote Debugging on Android (Legacy Workflow)](https://developer.chrome.com/devtools/docs/remote-debugging-legacy) up through step 3.3, but the TL;DR is install & run adb, enable USB debugging, then port forward 9222 from the device to the machine with Lighthouse.

+Run the cli with --disable-device-emulation=true configuration to disable device emulation.


don't need the =true here. Run the cli with --disable-device-emulation flag to disable device emulation.?

paulirish

I think we're done here once things from brendan and I are addressed. Excited about this!

paulirish · 2016-10-20T15:17:11Z

lighthouse-core/gather/drivers/driver.js

+      emulations.push(emulation.enableNetworkThrottling(this));
+    }
+
+    if (!flags.disableCpuThrottling) {


Let's make the cpu throttle disabled by default to start. Once the other stuff like #784 is all done we can flip that flag.

paulirish · 2016-10-20T15:18:42Z

readme.md

-  --list-trace-categories  Prints a list of all required trace categories and exits   [boolean]
-  --config-path            The path to the config JSON.
-  --perf                   Use a performance-test-only configuration                  [boolean]
+  --mobile                      Emulates a Nexus 5X                                  [default: true]


@wardpeet can you file an followup issue for our calibration-based throttling?

paulirish · 2016-10-20T15:22:41Z

lighthouse-core/test/gather/gather-runner-test.js

@@ -47,6 +47,31 @@ class TestGathererNoArtifact {

 const fakeDriver = require('./fake-driver');

+function getMockedEmulationDriver(deviceEmulation, networkThrottle, cpuThrottle) {


rename to emulationFn, netThrottleFn, cpuThrottleFn

paulirish · 2016-10-20T15:23:03Z

lighthouse-core/test/gather/gather-runner-test.js

+    };
+    let createEmulationCheck = variable => () => {
+      tests[variable] = true;
+


nuke empty line on this and the others

paulirish · 2016-10-20T15:29:28Z

lighthouse-core/test/gather/gather-runner-test.js

+    sendCommand(command) {
+      switch (command) {
+        case 'Network.emulateNetworkConditions':
+          return Promise.resolve(networkThrottle && networkThrottle());


Just wondering if we could do these a tad dryer..

case 'Network.emulateNetworkConditions': fn = netThrottleFn; // ... } return Promise.resolve(fn && fn());

wdyt?

wardpeet · 2016-10-24T06:09:43Z

will have some time today to address these issue, we had some storage crisis.

paulirish

yup this works for me. Thanks so much for making this happen!

will wait for @brendankenny brendan to +1 before landing..

paulirish · 2016-10-24T18:33:12Z

@wardpeet can you resolve the conflicts in preparation to land?

wardpeet · 2016-10-24T21:22:34Z

Yes will merge

brendankenny

looking good. I'll take a last look once the merge conflicts are resolved

brendankenny · 2016-10-25T00:39:14Z

lighthouse-cli/index.js

@@ -100,7 +104,7 @@ Example: --output-path=./lighthouse-results.html`
  .choices('output', Object.keys(Printer.OUTPUT_MODE))

  // default values
-  .default('mobile', true)
+  .default('disable-cpu-throttling', true)


this default should be in driver.beginEmulation() as well, since without it there, anyone using LH as a module will be cpu throttled

added to gather-runner

brendankenny · 2016-10-25T00:40:38Z

readme.md

 ```

 ## Lighthouse w/ mobile devices

 Lighthouse can run against a real mobile device. You can follow the [Remote Debugging on Android (Legacy Workflow)](https://developer.chrome.com/devtools/docs/remote-debugging-legacy) up through step 3.3, but the TL;DR is install & run adb, enable USB debugging, then port forward 9222 from the device to the machine with Lighthouse.

+Run the cli with --disable-device-emulation configuration to disable device emulation.


it seems like you'd want to run with all three --disable-* flags when running on a real device?

Not always because you might still need network throttling. And cpu throttling is disabled by default.

wardpeet · 2016-10-25T04:51:06Z

Will have to cleanup gather-runner-test in another PR with the connection Class. But for now the tests work again

googlebot · 2016-10-26T16:21:30Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

wardpeet · 2016-10-26T16:26:36Z

@paulirish thanks :)

paulirish · 2016-10-26T16:33:55Z

Without cpu throttling:

With cpu throttling:

paulirish · 2016-10-26T16:50:24Z

Landing! Thanks for the hookup on this, @wardpeet!
_

Keep in mind that CPU throttling is landing but disabled by default until proper UI is added. (#568, #784)

If you want to use it... lighthouse --disable-cpu-throttling=false ...

Keep in mind that CPU throttling now available but disabled by default until proper UI is added. (GoogleChrome#568, GoogleChrome#784) If you want to use it: `--disable-cpu-throttling=false`

addyosmani reviewed Oct 5, 2016

View reviewed changes

paulirish reviewed Oct 5, 2016

View reviewed changes

wardpeet force-pushed the feature/cpu-throttle branch from 548b01c to 3083efc Compare October 11, 2016 05:41

paulirish reviewed Oct 11, 2016

View reviewed changes

wardpeet force-pushed the feature/cpu-throttle branch from 96ac9fa to c1a9be3 Compare October 14, 2016 06:46

wardpeet added 4 commits October 16, 2016 11:42

Add cpu throttle

56bc5e0

Add extra flags to cli to disable emulation

ea27468

disable-device-emulation, disable-cpu-throttling, disable-network-throttling

Nuke --mobile flag

354e531

Update emulation tests

75e44b7

wardpeet force-pushed the feature/cpu-throttle branch from c1a9be3 to 75e44b7 Compare October 16, 2016 09:42

addyosmani assigned paulirish Oct 17, 2016

brendankenny requested changes Oct 20, 2016

View reviewed changes

paulirish reviewed Oct 20, 2016

View reviewed changes

paulirish mentioned this pull request Oct 21, 2016

Convert CLI to 100% typescript #702

Merged

wardpeet mentioned this pull request Oct 24, 2016

Calibration for cpu throttling #820

Closed

wardpeet added 2 commits October 24, 2016 17:15

review fixes @brendankenny

39d6bb9

review fixes @paulirish

dd6f929

paulirish approved these changes Oct 24, 2016

View reviewed changes

addyosmani approved these changes Oct 24, 2016

View reviewed changes

brendankenny requested changes Oct 25, 2016

View reviewed changes

Merge branch 'master' into feature/cpu-throttle

5a94ff0

wardpeet and others added 4 commits October 25, 2016 21:50

disableCpuThrottle is undefined so we set it to true

f60fe96

Merge branch 'master' into feature/cpu-throttle

01dc3d5

update paths after rename.

4cb1e87

update readme about --disable flags.

6aa6def

paulirish merged commit 5cf4c13 into master Oct 26, 2016

paulirish deleted the feature/cpu-throttle branch October 26, 2016 16:52

wardpeet mentioned this pull request Feb 26, 2017

Enable CPU throttling (4.5x) #1778

Merged

		@@ -47,6 +47,31 @@ class TestGathererNoArtifact {

		const fakeDriver = require('./fake-driver');

		function getMockedEmulationDriver(deviceEmulation, networkThrottle, cpuThrottle) {

Add cpu throttle #747

Add cpu throttle #747

Conversation

wardpeet commented Oct 5, 2016

wardpeet commented Oct 5, 2016

paulirish commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Oct 5, 2016 • edited by brendankenny Loading

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

Choose a reason for hiding this comment

wardpeet Oct 11, 2016 • edited Loading

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

Choose a reason for hiding this comment

wardpeet commented Oct 11, 2016

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

wardpeet commented Oct 14, 2016 • edited Loading

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

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

wardpeet Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish 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

Choose a reason for hiding this comment

wardpeet commented Oct 24, 2016

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Oct 24, 2016

wardpeet commented Oct 24, 2016

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

wardpeet Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

wardpeet commented Oct 25, 2016

googlebot commented Oct 26, 2016

wardpeet commented Oct 26, 2016

paulirish commented Oct 26, 2016

paulirish commented Oct 26, 2016

paulirish Oct 5, 2016 •

edited by brendankenny

Loading

wardpeet Oct 11, 2016 •

edited

Loading

wardpeet commented Oct 14, 2016 •

edited

Loading

wardpeet Oct 24, 2016 •

edited

Loading

wardpeet Oct 25, 2016 •

edited

Loading