-
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable throttling for non-performance passes #1740
Conversation
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 like the overall approach and love the speedup!
lighthouse-core/gather/driver.js
Outdated
@@ -673,21 +673,31 @@ class Driver { | |||
return this.sendCommand('Runtime.enable'); | |||
} | |||
|
|||
beginEmulation(flags) { | |||
beginEmulation(options) { |
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.
any reason to switch from flags to options? looks like this just uses flags
lighthouse-core/gather/driver.js
Outdated
const emulations = []; | ||
|
||
if (!flags.disableDeviceEmulation) { | ||
emulations.push(emulation.enableNexus5X(this)); | ||
if (!passConfig.useThrottling) { |
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.
this is one case I think having it nested would make it more readable, took me a bit to spot the early return (though that may be more diff noise and it's easier without the diff)
thoughts on the following?
if (useThrottling) {
if (!disableNetwork) emulations.push()
if (!disableCpu) emulations.push
} else {
emulations.push(...)
}
return Promise.all()
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.
so much better. done.
lighthouse-core/gather/driver.js
Outdated
return this.sendCommand('Network.enable') | ||
.then(_ => emulation.goOffline(this)) | ||
.then(_ => { | ||
this.online = 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.
nit: could probably one-line this too
lighthouse-core/gather/driver.js
Outdated
}); | ||
} | ||
|
||
setThrottling(cliFlags, passConfig) { |
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.
should passConfig just be a boolean, useThrottling
?
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.
Ah, see you're passing in the options.config
below. Guess it's fine.
12ae6de
to
d81792a
Compare
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.
PTAL. updated.
@patrickhulce good call inverting the conditional. much better now.
lighthouse-core/gather/driver.js
Outdated
const emulations = []; | ||
|
||
if (!flags.disableDeviceEmulation) { | ||
emulations.push(emulation.enableNexus5X(this)); | ||
if (!passConfig.useThrottling) { |
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.
so much better. done.
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
This is the approach that I ended up with after playing around a bit.
useThrottling
prop in the config for each passgoOnline
comes back it'll restore whatever throttling should be for that pass.setThrottling
method in driver does the work. the oldbeginEmulation
now defers to it for some parts.wdyt?