Skip to content
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

core(network-recorder): optimize network idle detection #9712

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented Sep 20, 2019

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
Copy link
Collaborator

connorjclark commented Sep 21, 2019

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

image

(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
Copy link
Member Author

nice! Looks super handy.

@brendankenny
Copy link
Member Author

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
Copy link
Member Author

cool :)

perf comparison

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice optimization! 🏁 🏎 💨💨

for (let i = 0; i < this._records.length; i++) {
const record = this._records[i];
if (!NetworkRecorder.isNetworkRecordFinished(record)) {
if (IGNORED_NETWORK_SCHEMES.includes(record.parsedURL.scheme)) continue;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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++;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sure, looks better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants