core(network-recorder): optimize network idle detection

[brendankenny]

The NetworkRecorder.recordsFromLogs performance described here was pretty bad, the vast majority of time spent in findNetworkQuietPeriods. Something like 1380ms spent on this is especially bad because it happens three times in a typical LH run: once right before GatherRunner.afterPass , once in a computed artifact for the first audit that requests networkRecords for the defaultPass, and once spread over the run of pass() where NetworkRecorder is essentially running recordsFromLogs as protocol events come in. That 3x becomes 6x if e.g. there's a valid offline site served by a SW.

The biggest issue was constructing a full set of quiet periods for the entire set of network records when really for networkIdle and network-2-idle we only care if there's currently a quiet period. That's equivalent to this check passing so it adds a final period ending at Infinity.

This PR adds a new internal method _isActiveIdlePeriod that does essentially that (also catching the ignored network schemes check). Behavior is maintained exactly as before.

In testing I've found a 75-90% drop in execution time, the more requests on a page makes for a bigger performance win (so maybe taking 3 seconds off that LH run on @paulirish's machine). We could probably knock off another 25% in a few ways, but they should wait for follow ups as they're more invasive and need to be considered more carefully than this equivalent transformation.

[connorjclark]

I have a script (https://github.com/GoogleChrome/lighthouse/blob/timings-script/lighthouse-core/scripts/timings.js) for measuring these things :)

(collected with node lighthouse-core/scripts/timings.js --name base --collect -n 10 --urls https://www.example.com https://www.nyt.com https://www.theverge.com/)

Looks good!

[brendankenny]

nice! Looks super handy.

[brendankenny]

Though if n is 30 for lh:gather:getDevtoolsLog that means it's treating the three calls per run as independent samples? That's going to throw things off since for most sites two of the three (offline and http redirect passes) are going to be basically zero. lh:computed:NetworkRecords should show the improvement more clearly, since we don't record computed artifact time when they're pulled from the cache.

The most straightforward fix to that might be to just sum them per run (so the number would be "the total amount of time spent in lh:gather:getDevtoolsLog for this run", not a record of individual calls) instead of treating them as independent samples.

[brendankenny]

cool :)

[patrickhulce]

nice optimization! 🏁 🏎 💨💨

[patrickhulce]

nesting/mixing and matching seems a bit harder to parse IMO than consistency on one

can I interest in you in

// Request isn't over network, so it doesn't count.
if (IGNORED_NETWORK_SCHEMES.includes(record.parsedURL.scheme)) continue;
// Request finished, so it's not inflight.
if (NetworkRecorder.isNetworkRecordFinished(record))) continue;
// Request is inflight.
inflightRequests++;

or

if (!NetworkRecorder.isNetworkRecordFinished(record) && 
    !IGNORED_NETWORK_SCHEMES.includes(record.parsedURL.scheme)) {
  inflightRequests++;
}

[brendankenny]

nesting/mixing and matching seems a bit harder to parse IMO than consistency on one

sure, looks better