fix: remove afterPass throttling #1901
Conversation
|
@brendankenny does this address your concern from #1868? |
There was a problem hiding this comment.
looks good.
I was not a fan of this idea when I originally wrote that comment--seemed too easy to accidentally write code that you think should be throttled but won't be--but thinking about it since, it makes a certain amount of sense that just like afterPass is after tracing that it also be after throttling.
It does mean that a gatherer can't be throttled and avoid the overhead of tracing (depending on which pass it ends up in), but really the only reason you'd want to be throttled is to measure the effect of throttling on some normal action, so I'd imagine the need for that would be rare. We can always add a new pass stage if we ever somehow need that.
This also makes me think throttling should also be applied after beforePass runs for symmetry and easy reasoning about gatherer life cycles, but we can wait on that.
lighthouse-core/gather/driver.js
Outdated
| const useCpuThrottling = passConfig.useThrottling && !flags.disableCpuThrottling; | ||
| const useNetworkThrottling = passConfig.useThrottling && !flags.disableNetworkThrottling; | ||
| const cpuThrottleFunc = `${useCpuThrottling ? 'enable' : 'disable'}CPUThrottling`; | ||
| const networkThrottleFunc = `${useNetworkThrottling ? 'enable' : 'disable'}NetworkThrottling`; |
There was a problem hiding this comment.
string construction is pretty bad, but so was the original :P
Short of a refactor of emulation.js, what about
setThrottling(flags, passConfig) {
const throttleCpu = passConfig.useThrottling && !flags.disableCpuThrottling;
const cpuPromise = throttleCpu ? emulation.enableCPUThrottling(this) :
emulation.disableCPUThrottling(this);
const throttleNetwork = passConfig.useThrottling && !flags.disableNetworkThrottling;
const networkPromise = throttleNetwork ? emulation.enableNetworkThrottling(this) :
emulation.disableCPUThrottling(this);
return Promise.all([cpuPromise, networkPromise]);
}| @@ -244,6 +244,9 @@ class GatherRunner { | |||
| log.verbose('statusEnd', status); | |||
| }); | |||
|
|
|||
| // Disable throttling so the afterPass analysis isn't throttled | |||
| pass = pass.then(_ => driver.setThrottling(options.flags, {useThrottling: false})); | |||
There was a problem hiding this comment.
can you update the top outline to mention this as well? endTrace (if requested) & endNetworkCollect & endThrottling or something?
|
any reason appveyor keeps failing? |
|
|
|
@brendankenny yes everything should be failing that grabs canary chrome, hence #1911 |
|
oh right, whoops, was thinking of the one that landed in 1.6.0 |
R: @brendankenny
fixes #1868 (review)