perf: make network quiet threshold configurable per pass #2220

patrickhulce · 2017-05-10T23:25:11Z

in the name of OYB, shaves off ~9 seconds from all runs with the default passes

renames pauseAfterLoad to the more appropriate networkQuietThreshold, previously the flag was for speeding up the tests and was not documented in the CLI

paulirish

epic fix. nice work!

paulirish · 2017-05-11T00:31:36Z

lighthouse-core/config/default.js

@@ -29,6 +30,7 @@ module.exports = {
  {
    "passName": "offlinePass",
    "useThrottling": false,
+    "networkQuietThresholdMs": 500,


do we even need to wait 500 on these two passes?

yeah probably not we really just need the main page, right?

brendankenny · 2017-05-11T00:36:09Z

wait, this was only 5000 because of #2023 :P It was 500 before

brendankenny · 2017-05-11T00:50:41Z

This explains the end of trace ending up 10 seconds after most of the other metrics had completed, since we have the double 5 second wait.

Can we eliminate one of networkQuietThresholdMs or pauseBeforeTraceEndMs while we're at it? They're almost exactly the same, the only difference is it's

network quiet or onload
wait networkQuietThresholdMs
run all gatherer.pass()es
wait pauseBeforeTraceEndMs
end trace

It doesn't seem like both are necessary. Arguably networkQuietThresholdMs could go in the position of pauseBeforeTraceEndMs, because if a gatherer is doing anything in its pass method in a pass with recordTrace, it wants its activities to be recorded by the trace and delay TTI or whatever is being run on that pass.

patrickhulce · 2017-05-11T00:56:42Z

Can we eliminate one of networkQuietThresholdMs or pauseBeforeTraceEndMs while we're at it?

The answer is how much TTCI failure are we willing to tolerate? Most sites should be fine without pauseBeforeTraceEnd, but there are definite cases where it will fail. Maybe we make it optional?

brendankenny · 2017-05-11T01:02:40Z

for the default defaultPass, at least, there aren't any gatherers with a pass() method, so the actual execution is

network quiet or onload
wait networkQuietThresholdMs
wait pauseBeforeTraceEndMs
end trace

so we can just get rid of one and wait the sum of the current two, if we need that long

patrickhulce · 2017-05-11T03:16:55Z

so we can just get rid of one and wait the sum of the current two, if we need that long

It's not quite that simple, we can consolidate for sure but they serve different purposes. Increasing networkQuietThreshold to 10s might cause many more timeouts (it's not just waiting 5s at the end but up to 5s between each request) than the current approach, but adding 5s to pauseBeforeTraceEnd would also fail to capture network quiet periods more frequently than the current approach.

networkQuietThreshold at 5s is just a requirement of TTCI and we'll have to suffer through if we want to reliably determine it (though we could split into a 5s 2-inflight requirement and 500ms 0-inflight requirement).

Given networkQuietThreshold fixed at 5s, pauseBeforeTraceEnd should really be interpreted as "what's the longest we might expect a reasonable page to have main thread activity after all network requests have finished" Example: if the last network request finishes and then there's a single 50ms long task, we don't want that to make TTCI undeterminable. 5 seconds is quite generous, but 2s of main thread isn't that unheard of so we can play around with the number here.

No matter what we can definitely skip pauseBeforeTraceEnd in the timeout case which is the current worst case scenario and shave off 5s.

brendankenny · 2017-05-11T06:48:29Z

I think I see what you're saying. Basically we want five seconds of network quiet, but in theory the first few seconds of that window could contain a long task, so we need to wait longer?

If I understand you correctly, there's not a ton of difference in waiting for seven seconds of network quiet. A single network request could go out after five seconds cancelling the network quiet detection but before the CPU quieted down, but with the current scheme three network requests going out after networkQuietThreshold but less than five seconds after CPU quiet would have the same effect. Three new requests is less likely than one but it's not great.

Basically this is all hand wavey and a post hoc justification and maybe that's what smells about it :) The fact that we don't have a way to observe the main thread is the primary issue with finding a good solution. If networkQuietThreshold was really all about TTCI, it would be listening for dropping down to two, not zero, network requests for five seconds. What it was originally for was that was our best guess at "loaded" when we wrote it (better than onload + pause), and now it's functioning as a clumsy heuristic for CPU quiet. "5s 2-inflight requirement and 500ms 0-inflight requirement" suffers from the same issue.

For now, how do you feel about renaming pauseBeforeTraceEnd to pauseAfterNetworkQuiet or whatever (theLimitOfOurPatienceWaitingForAPossibleLongTask) and moving it out of endTrace and into gather-runner where it belongs (it doesn't make sense as an option to ending the trace and it delays everything afterwards, not just the ending the trace, so might as well make it prominent).

brendankenny · 2017-05-11T06:57:54Z

We should also consider (in the future) upping _waitForNetworkIdle to wait for one request (and maybe even two) and see its effect on average runtime. We've suspected there are a number of sites that hit the 25s timeout that don't need to because they have a single request open (Does FMP still allow one(?) open request when declaring the page ready and the last candidate official?). Many sites might not budge, but average runtime could be decreased significantly.

and we should add time to run lighthouse to plots/ cause we don't know what the average runtime is :)

patrickhulce · 2017-05-11T15:12:48Z

For now, how do you feel about renaming pauseBeforeTraceEnd to pauseAfterNetworkQuiet or whatever (theLimitOfOurPatienceWaitingForAPossibleLongTask) and moving it out of endTrace

ya let's do it

patrickhulce · 2017-05-11T16:29:54Z

OK lots of new changes :)

I've split the thresholds into 3 to accurately track what they're trying to do and moved it all into the page load

network-2-idle and network-2-busy have been added to network recorder
pauseAfterLoad has been split into pauseAfterLoad and pauseAfterNetworkQuiet to appropriately account for the fact that in networkQuiets case we've already had 5s of 2-quiet silence, so we probably don't need another full 5
networkQuietThreshold is used as the debounce time for network-2-idle instead of abusing pauseAfterLoad
There's no more pauseBeforeTraceEnd, it's intention is captured by pauseAfterLoad and pauseAfterNetworkQuiet

patrickhulce · 2017-05-11T16:31:21Z

lighthouse-core/gather/driver.js


 const _uniq = arr => Array.from(new Set(arr));

 class Driver {
  static get MAX_WAIT_FOR_FULLY_LOADED() {
-    return 25 * 1000;
+    return 30 * 1000;


increased to account for the fact that the trace will end abruptly after this time instead of 5s after it, so we still aren't capturing any larger traces than 30s

paulirish

nice. i like the precision with the new options.

paulirish · 2017-05-13T18:07:54Z

lighthouse-core/gather/driver.js

@@ -25,13 +25,15 @@ const URL = require('../lib/url-shim');
 const log = require('../lib/log.js');
 const DevtoolsLog = require('./devtools-log');

-const PAUSE_AFTER_LOAD = 5000;
+const DEFAULT_PAUSE_AFTER_LOAD = 0;


can you add comments above these to explain what they are for? otherwise folks will have to backtrack to this discussion to totally understand. :)

perf: make network quiet threshold configurable per pass

4506fc4

patrickhulce added the OYB label May 10, 2017

paulirish approved these changes May 11, 2017

View reviewed changes

lower it more

3077481

split into 3 thresholds

e47cae6

patrickhulce commented May 11, 2017

View reviewed changes

Merge branch 'master' into configurable_network_quiet_threshold

dce78ea

paulirish mentioned this pull request May 11, 2017

Operation Yaquina Bay #2146

Closed

paulirish approved these changes May 13, 2017

View reviewed changes

paulirish reviewed May 13, 2017

View reviewed changes

add comments

8820419

patrickhulce merged commit d99b5ad into master May 13, 2017

patrickhulce deleted the configurable_network_quiet_threshold branch May 13, 2017 18:43

patrickhulce mentioned this pull request May 16, 2017

polish: listen for network idle after DCL #2271

Merged

brendankenny mentioned this pull request Oct 3, 2017

fix: update plots-config to use newer quiet parameters #3464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: make network quiet threshold configurable per pass #2220

perf: make network quiet threshold configurable per pass #2220

patrickhulce commented May 10, 2017

paulirish left a comment

paulirish May 11, 2017

patrickhulce May 11, 2017

brendankenny commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017 •

edited

Loading

brendankenny commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017

patrickhulce commented May 11, 2017

patrickhulce May 11, 2017

paulirish left a comment

paulirish May 13, 2017

patrickhulce May 13, 2017

perf: make network quiet threshold configurable per pass #2220

perf: make network quiet threshold configurable per pass #2220

Conversation

patrickhulce commented May 10, 2017

paulirish left a comment

Choose a reason for hiding this comment

paulirish May 11, 2017

Choose a reason for hiding this comment

patrickhulce May 11, 2017

Choose a reason for hiding this comment

brendankenny commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017 • edited Loading

brendankenny commented May 11, 2017

brendankenny commented May 11, 2017

patrickhulce commented May 11, 2017

patrickhulce commented May 11, 2017

patrickhulce May 11, 2017

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish May 13, 2017

Choose a reason for hiding this comment

patrickhulce May 13, 2017

Choose a reason for hiding this comment

patrickhulce commented May 11, 2017 •

edited

Loading