-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Enable CPU throttling (4.5x) #1778
Conversation
cc @wardpeet in case I'm missing anything. |
@addyosmani how much closer does this take us to aligning with WPT results? Curious if you had time to run the same site against WPT and this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks for this.
@@ -313,9 +313,8 @@ class GatherRunner { | |||
return Promise.reject(new Error('You must provide a config')); | |||
} | |||
|
|||
// CPU throttling is temporarily off by default | |||
if (typeof options.flags.disableCpuThrottling === 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik you can nuke this conditional
@@ -124,7 +124,7 @@ function onGenerateReportButtonClick(background, selectedAggregations) { | |||
|
|||
background.runLighthouseInExtension({ | |||
flags: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop this flags object
Oh I think there's a note about mobile testing in the readme that can also use a tweak. |
@paulirish Woo. Thanks for the review! I'll get those three bits dropped shortly. @ebidel Good questions :) The 3.5x number came from locally comparing traces seen on a Moto G4 over remote debugging vs LH on desktop. Talking to Pat this week, he seemed supportive of the change getting us 90% of the way there in the short term. I'm going to run some more experiments as a sanity check and report back here. |
@paulirish relevant to the multiplier: do we have any UMA data on the type of desktop/hardware the average DevTools user is on? Just want a sanity check that the MBP is a fair baseline to assume. |
After trying to look at calibration for a while, I'm updating my recommendation to CPU throttling at 4.5x and tweaking that over time. @paulirish could you let me know if you'd be down with that and I'll update my PR? Here are a few comparisons between WPT runs over a Moto G4 with 3G and DevTools using a 4.3-4.5x CPU throttling + custom network throttling closer to what WPT is doing: Polymer Shop - closer with 4.5x CPU slowdown ReactHN - closer with a 4.5x CPU throttling slowdown Fashion United - close..ish with 4.5x CPU slowdown Voice Memos with 4.5x Code.gov with 4.3x CPU slowdown Twitter.com (outlier) at 4x CPU (~1000ms slower) |
@paulirish I believe I've addressed your feedback and rebased against master. PTAL whenever you get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes i think.
We also discussed yesterday an easy mode for calibration. I think it went something like this:
- Expose the default multiplier via UI/CLI, etc.
- Communicate that you are uncalibrated
- Link to docs on doing a calibration
- That has a series of manual steps which could include running some npm module or whatever.
- It ends up giving you a new multiplier for your machine
- You enter that into the UI/CLI and the setting is persisted.
Anyway. we can head into this later. I'm happy to get the static multiplier in place now. :)
lighthouse-cli/bin.ts
Outdated
@@ -124,8 +124,7 @@ Example: --output-path=./lighthouse-results.html`, | |||
// default values | |||
.default('chrome-flags', '') | |||
.default('disable-cpu-throttling', false) | |||
.default('output', Printer.GetValidOutputOptions()[Printer.OutputMode.none]) | |||
.default('output-path', 'stdout') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think you meant to change this, right?
lighthouse-cli/bin.ts
Outdated
@@ -124,8 +124,7 @@ Example: --output-path=./lighthouse-results.html`, | |||
// default values | |||
.default('chrome-flags', '') | |||
.default('disable-cpu-throttling', false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false => true ?
This change enables CPU throttling by default and switches the slow-down from 5x -> 3.5x (as discussed with @paulirish in person). The 3.5x aligns closer with the CPU differences observed between a MBP and a Moto G4 (our current recommendation for 'average' mobile hardware).
If there are any further changes required to enable this, please feel free to 馃憢