feat: add consistently interactive audit #2023

patrickhulce · 2017-04-17T17:37:05Z

https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/preview

patrickhulce · 2017-04-21T21:32:40Z

PTAL :)

paulirish

only minor adjustments here. lgtm % those noted.

paulirish · 2017-04-24T23:55:44Z

lighthouse-core/audits/consistently-interactive.js

+   */
+  static _findNetworkQuietPeriods(networkRecords, traceOfTab) {
+    const traceEnd = traceOfTab.timestamps.traceEnd;
+    const timeBoundaries = networkRecords.reduce((boundaries, record) => {


forEach instead plz.

can you add a comment to clarify these timeBoundaries are just the timestamps of when requests start and end

paulirish · 2017-04-25T00:16:47Z

lighthouse-core/audits/consistently-interactive.js

+    let inflight = 0;
+    let quietPeriodStart = 0;
+    const quietPeriods = [];
+    timeBoundaries.forEach(boundary => {


hey a forEach! 😁 😍

paulirish · 2017-04-25T00:19:15Z

lighthouse-core/audits/consistently-interactive.js

+    const quietPeriods = [];
+    timeBoundaries.forEach(boundary => {
+      if (boundary.isStart) {
+        // we are exiting a quiet period


// we've just started a new request. are we exiting a quiet period?

paulirish · 2017-04-25T00:22:23Z

lighthouse-core/audits/consistently-interactive.js

+        inflight++;
+      } else {
+        inflight--;
+        // we are entering a quiet period


// we've just completed a request. are we entering a new quiet period?

paulirish · 2017-04-25T00:22:50Z

lighthouse-core/audits/consistently-interactive.js

+      return boundaries;
+    }, []).sort((a, b) => a.time - b.time);
+
+    let inflight = 0;


s/inflight/inflightReqs/

done-ish :)

paulirish · 2017-04-25T00:26:34Z

lighthouse-core/audits/consistently-interactive.js

+    const FMPTsInMs = traceOfTab.timestamps.firstMeaningfulPaint;
+
+    const networkQuietPeriods = this._findNetworkQuietPeriods(networkRecords, traceOfTab)
+        .filter(period => period.end > FMPTsInMs + REQUIRED_QUIET_WINDOW &&


extract inner fn to const?

paulirish · 2017-04-25T00:26:54Z

lighthouse-core/audits/consistently-interactive.js

+        .filter(period => period.end > FMPTsInMs + REQUIRED_QUIET_WINDOW &&
+            period.end - period.start >= REQUIRED_QUIET_WINDOW);
+    const cpuQuietPeriods = this._findCPUQuietPeriods(longTasks, traceOfTab)
+        .filter(period => period.end > FMPTsInMs + REQUIRED_QUIET_WINDOW &&


and reuse here

brendankenny

meta:

should this be in trace-of-tab?
if this is what we're going to be pushing at I/O, should the various audits switching over to firstInteractive from TTI in feat: add firstInteractive #2013 be switching to this instead?

brendankenny · 2017-04-25T00:51:48Z

should this be in trace-of-tab?

er, that would be should this be in pwmetrics-events :)

patrickhulce · 2017-04-25T16:47:49Z

should this be in pwmetrics-events?

yep, added 👍

if this is what we're going to be pushing at I/O, should the various audits switching over to firstInteractive from TTI in #2013 be switching to this instead?

sadly, yes. I replaced it with firstInteractive since it's most comparable to our previous definition in spirit compared to the pessimism of this one, but we'll probably want to switch the load fast enough for PWA one sometime before I/O if that policy sticks, it has unfortunate effects for other audits like the unused images since by definition consistently interactive would wait for the images to finish, so I'm not sold on switching everything just yet...

patrickhulce · 2017-05-01T17:30:07Z

rebased and ready to take on the day 🌥 -> 🌤

patrickhulce · 2017-05-01T17:30:47Z

waiting for #2062 to add a real trace test

brendankenny

review!

brendankenny · 2017-05-04T22:45:37Z

lighthouse-core/audits/consistently-interactive.js

+//   https://www.desmos.com/calculator/uti67afozh
+const SCORING_POINT_OF_DIMINISHING_RETURNS = 1700;
+const SCORING_MEDIAN = 10000;
+const SCORING_TARGET = 5000;


brendankenny · 2017-05-04T22:46:01Z

lighthouse-core/audits/consistently-interactive.js

+      description: 'Consistently Interactive (beta)',
+      helpText: 'The point at which most network resources have finished loading and the ' +
+          'CPU is idle for a prolonged period.',
+      optimalValue: SCORING_TARGET.toLocaleString() + 'ms',


brendankenny · 2017-05-04T22:52:55Z

lighthouse-core/audits/consistently-interactive.js

+        }
+
+        const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace)
+            .filter(event => event.end - event.start >= 50);


event.duration

brendankenny · 2017-05-04T22:54:08Z

lighthouse-core/audits/consistently-interactive.js

+          throw new Error('No firstMeaningfulPaint found in trace.');
+        }
+
+        const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace)


pass in FMP here to avoid computing stuff before then? (also would allow removal of FMP stuff in isLongEnoughQuietPeriod above)

though I guess _findNetworkQuietPeriods would then also need to find out and clip by FMP

wouldn't really be able to remove anything since we still need to check that the period is 5s after FMP (even if long tasks are ignored before FMP) so it's just the saved long tasks we're iterating through, and IMO it makes the logic harder to parse that FMP has already been dealt with in CPU but we still have to deal with it in network, fine leaving?

fine leaving?

yeah, I guess it's just awkward one way or the other. SG

brendankenny · 2017-05-04T22:57:06Z

lighthouse-core/audits/consistently-interactive.js

+
+    return Promise.all(computedTraceArtifacts)
+      .then(([traceModel, traceOfTab]) => {
+        if (!traceOfTab.timestamps.firstMeaningfulPaint) {


should probably throw on lack of DCL too (as in TTFI)

(though in the spec I'm not actually sure this is true, it just seemed like the proper thing to do. I should raise it)

(though in the spec I'm not actually sure this is true, it just seemed like the proper thing to do. I should raise it)

wouldn't no FMP be a bug in Chrome/Lighthouse? Unless you mean something else

I was talking about the fact that we're doing the max of DCL and the value. I wasn't sure that's in the spec or if I added that to be sure it doesn't happen before TTFI

brendankenny · 2017-05-09T00:52:39Z

lighthouse-core/audits/consistently-interactive.js

+  }
+
+  /**
+   * @param {!Array<{start: number, end: number}>} longTasks


jsdoc description

brendankenny · 2017-05-09T00:52:46Z

lighthouse-core/audits/consistently-interactive.js

+  }
+
+  /**
+   * @param {!Array<{start: number, end: number}>} longTasks


jsdoc description :)

brendankenny · 2017-05-09T00:55:22Z

lighthouse-core/audits/consistently-interactive.js

+   * @param {!Array<{start: number, end: number}>} longTasks
+   * @param {{timestamps: {navigationStart: number, firstMeaningfulPaint: number,
+   *    traceEnd: number}}} traceOfTab
+   * @return {!Object}


would be great to have a type here :)

brendankenny · 2017-05-09T01:04:44Z

lighthouse-core/audits/consistently-interactive.js

+    const timeBoundaries = [];
+    networkRecords.forEach(record => {
+      const scheme = record.parsedURL && record.parsedURL.scheme;
+      if (scheme === 'data' || scheme === 'ws') {


might be overkill, but should we convert this to a ignoredNetworkSchemes array and pull it into a constant?

brendankenny · 2017-05-09T01:13:29Z

lighthouse-core/audits/consistently-interactive.js

+          traceOfTab.timestamps.domContentLoaded
+        ) * 1000;
+        const timeInMs = (timestamp - traceOfTab.timestamps.navigationStart * 1000) / 1000;
+        const extendedInfo = Object.assign(quietPeriodInfo, {timestamp, timeInMs});


shape of quietPeriodInfo is difficult to track down. Can it be made more evident here?

added typedef to findOverlappingQuietPeriods 👍

that is...not a terribly useful type definition :P If I wanted to use that extendedInfo I would have to check the param signatures of _findCPUQuietPeriods and _findNetworkQuietPeriods and read findOverlappingQuietPeriods to make sure there's only one code escape path. We don't have a great place for documenting artifact shapes, but it would be nice to always do it, even if in an awkward place

added a TimePeriod typedef

/** * @typedef {{ * start: number, * end: number, * }} */ let TimePeriod; // eslint-disable-line no-unused-vars

paulirish · 2017-05-09T19:32:02Z

I want this guy in ASAP, but not going to rush it. :)
@brendankenny your move here.

patrickhulce · 2017-05-09T21:17:25Z

FYI @brendankenny I split that part out into a separate PR so it's easier to review, PTAL #2197

googlebot · 2017-05-09T21:17:28Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

brendankenny · 2017-05-09T21:18:57Z

FYI @brendankenny I split that part out into a separate PR

🎉

patrickhulce · 2017-05-09T22:47:47Z

rebased and ready to go 💥

brendankenny

last documentation nits. PR looks great.

brendankenny · 2017-05-10T02:15:06Z

lighthouse-core/audits/consistently-interactive.js

+          traceOfTab.timestamps.domContentLoaded
+        ) * 1000;
+        const timeInMs = (timestamp - traceOfTab.timestamps.navigationStart * 1000) / 1000;
+        const extendedInfo = Object.assign(quietPeriodInfo, {timestamp, timeInMs});


that is...not a terribly useful type definition :P If I wanted to use that extendedInfo I would have to check the param signatures of _findCPUQuietPeriods and _findNetworkQuietPeriods and read findOverlappingQuietPeriods to make sure there's only one code escape path. We don't have a great place for documenting artifact shapes, but it would be nice to always do it, even if in an awkward place

brendankenny · 2017-05-10T02:16:35Z

lighthouse-core/audits/consistently-interactive.js

+  /**
+   * Finds all time periods where the number of inflight requests is less than or equal to the
+   * number of allowed concurrent requests (2).
+   * @param {!Array<WebInspector.NetworkRequest>} networkRecords


nit: !Array<!WebInspector.NetworkRequest>

brendankenny · 2017-05-10T02:17:29Z

lighthouse-core/audits/consistently-interactive.js

+  /**
+   * Finds the first time period where a network quiet period and a CPU quiet period overlap.
+   * @param {!Array<{start: number, end: number}>} longTasks
+   * @param {!Array<WebInspector.NetworkRequest>} networkRecords


!Array<!WebInspector.NetworkRequest>

feat: add consistently interactive audit

0a2fe77

patrickhulce force-pushed the consistently_interactive branch from 2d45f84 to 0a2fe77 Compare April 17, 2017 18:01

fix comments

3cce886

paulirish mentioned this pull request Apr 25, 2017

feat: add firstInteractive #2013

Merged

paulirish approved these changes Apr 25, 2017

View reviewed changes

brendankenny requested changes Apr 25, 2017

View reviewed changes

feedback

19f1aef

This was referenced Apr 26, 2017

Add audit to check if start_url is cached by SW #2040

Merged

Move networkRecords to a computed artifact #2062

Closed

patrickhulce added 2 commits May 1, 2017 09:03

Merge branch 'master' into consistently_interactive

464df61

update dbw trace

62e105d

patrickhulce added 2 commits May 5, 2017 13:04

Merge branch 'master' into consistently_interactive

842b76a

add fixture test

7dc9123

brendankenny requested changes May 9, 2017

View reviewed changes

patrickhulce and others added 3 commits May 8, 2017 20:29

feedback

eb10cb9

add support for unfinished network records

a5d62b8

Merge branch 'master' into consistently_interactive

29c67f9

patrickhulce added 3 commits May 9, 2017 13:43

Merge branch 'master' into consistently_interactive

16b22a9

Merge branch 'master' into consistently_interactive

1feccc7

finish all network records

5f077ae

patrickhulce mentioned this pull request May 9, 2017

refactor: makes non-finished network records available #2197

Merged

Merge branch 'master' into consistently_interactive

9505527

brendankenny approved these changes May 10, 2017

View reviewed changes

add typedef

1f51140

brendankenny merged commit d2cb5a2 into master May 10, 2017

brendankenny deleted the consistently_interactive branch May 10, 2017 17:24

brendankenny mentioned this pull request May 11, 2017

perf: make network quiet threshold configurable per pass #2220

Merged

patrickhulce mentioned this pull request Jun 15, 2021

core(total-byte-weight): count partially finished requests #12665

Merged

feat: add consistently interactive audit #2023

feat: add consistently interactive audit #2023

Conversation

patrickhulce commented Apr 17, 2017 • edited by brendankenny Loading

patrickhulce commented Apr 21, 2017

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Apr 25, 2017 • edited Loading

patrickhulce commented Apr 25, 2017

patrickhulce commented May 1, 2017

patrickhulce commented May 1, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 9, 2017

patrickhulce commented May 9, 2017

googlebot commented May 9, 2017

brendankenny commented May 9, 2017

patrickhulce commented May 9, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 17, 2017 •

edited by brendankenny

Loading

brendankenny commented Apr 25, 2017 •

edited

Loading