feat: add firstInteractive #2013

patrickhulce · 2017-04-14T21:07:10Z

Adds firstInteractive as defined here. Also changes all references from TTI to first-interactive. Removing TTI and replacing it with visually ready is in a different pending PR (currently a blocking bug in speedline I'm investigating)

patrickhulce · 2017-04-14T21:10:33Z

lighthouse-core/gather/computed/trace-of-tab.js

@@ -88,7 +88,10 @@ class TraceOfTab extends ComputedArtifact {
      firstMeaningfulPaint = lastCandidate;
    }

-    const onLoad = keyEvents.find(e => e.name === 'MarkLoad' && e.ts > navigationStart.ts);
+    const onLoad = frameEvents.find(e => e.name === 'loadEventStart' && e.ts > navigationStart.ts);


@paulirish MarkLoad was not onload in most cases I saw during my investigations, what's supposed to be the difference between it and loadEventStart?

markload should actually line up with loadEventEnd. if its not, then it must be selecting the wrong frame. Happy to have it change.

oh so it is fired per frame afterall then 👍

paulirish

this is so great. excited.

I want to make sure we understand how much this changes things, especially for the PWA test. Can you point to some data to clarify what the delta in TTIvAlpha and TTFIvBeta is?

also, curious if we should validate that this FI matches the results from https://codereview.chromium.org/2610183003 ?

paulirish · 2017-04-15T00:02:50Z

lighthouse-core/lib/traces/pwmetrics-events.js

@@ -131,39 +131,15 @@ class Metrics {
        }
      },
      {
-        name: 'Time to Interactive (vAlpha)',
-        id: 'tti',
+        name: 'First Interactive (vAlpha)',


paulirish · 2017-04-15T00:08:03Z

lighthouse-core/gather/computed/computed-artifact.js

   * @return {!Promise}
   */
-  request(artifact) {
+  request(artifact, artifacts) {


pretty interesting, but I like it. leads to more clean implementations.

can you rename to allArtifacts to clarify, since we have 3 diff sorts in here.

the problem is that it breaks the cache and requiredArtifacts dependency checks, since the computed artifact could theoretically access any artifact it wanted at this point. In this case first-interactive.js is only accessing other computed artifacts, so it's fine, but we can't guarantee it (here or in future computed artifacts)

I have a proposal I've been thinking about for a while (it also makes typing nicer for the artifacts object). I'll throw it together quickly and put it in a PR

Does this proposal allow for passing more than one artifact? I wanted to make consistently interactive a computed artifact too, but it needs trace + networkRecords

Does this proposal allow for passing more than one artifact?

No, but we definitely need that too. I can add it in a followup PR (or you can if you want). Basically computedArtifact's _cache needs to support a tuple as a key instead of just objects directly. There's no really efficient way to do that in JS today, but luckily these caches all have like one item in them :)

paulirish · 2017-04-15T00:09:18Z

lighthouse-core/audits/first-interactive.js

+
+// Parameters (in ms) for log-normal CDF scoring. To see the curve:
+//   https://www.desmos.com/calculator/uti67afozh
+const SCORING_POINT_OF_DIMINISHING_RETURNS = 1700;


presuming these are spitballed?

yeah I did it based on my hypothetical grading rubric from A+ -> F, each letter gets 20 point range with the median (C) being "passing" but not good. By this scale, A+ is basically the point of diminishing returns and A- extends to around 4 seconds.

paulirish · 2017-04-15T00:13:40Z

lighthouse-core/audits/load-fast-enough-for-pwa.js


      const extendedInfo = {
        formatter: Formatter.SUPPORTED_FORMATS.NULL,
-        value: {areLatenciesAll3G, allRequestLatencies, isFast, timeToInteractive}
+        value: {areLatenciesAll3G, allRequestLatencies, isFast, ttfiValue}


let's call it the full timeToFirstInteractive

paulirish · 2017-04-15T00:16:32Z

lighthouse-core/gather/computed/first-interactive.js

+const MAX_QUIET_WINDOW_SIZE = 5000;
+const TRACE_BUSY_MSG = 'trace was busy the entire time';
+
+class FirstInteractive extends ComputedArtifact {


lets add a fileoverview or other description that links to this doc and briefly describes wtf this does
https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/edit#

👍 moved it up from compute_ and added some detail

paulirish · 2017-04-15T00:40:12Z

lighthouse-core/gather/computed/trace-of-tab.js

-    const onLoad = keyEvents.find(e => e.name === 'MarkLoad' && e.ts > navigationStart.ts);
+    const onLoad = frameEvents.find(e => e.name === 'loadEventStart' && e.ts > navigationStart.ts);
+    const domContentLoaded = frameEvents.find(
+      e => e.name === 'domContentLoadedEventStart' && e.ts > navigationStart.ts


for both this and loadEvent, usually the End is used, but in this case, it actually makes more sense for us to focus on the browser's POV of these events... so 👍 on using Start

Actually......

The doc uses dclEnd rather than dclStart.
Since it's used as fi = Math.max(DCL, FI)... the End value seems much more accurate. (Handlers are setup during DCL, and the main thread is unavailable during this time)

yeah originally I was using DCL as the start point for search when using start made some sense, but now that it's just the actual firstInteractive point, using end is much better

paulirish · 2017-04-15T17:49:42Z

FYi I just finished a branch that moves networkRecords into a computed artifact. Shouldn't matter much for this though.

brendankenny · 2017-04-17T07:33:21Z

FYi I just finished a branch that moves networkRecords into a computed artifact. Shouldn't matter much for this though.

I'm not sure how this would work, so interested in seeing it :)

patrickhulce · 2017-04-21T21:38:21Z

rebased PTAL :)

paulirish

Mostly looking great. A few small adjustments..

paulirish · 2017-04-24T23:04:02Z

lighthouse-core/gather/computed/trace-of-tab.js

-    const onLoad = keyEvents.find(e => e.name === 'MarkLoad' && e.ts > navigationStart.ts);
+    const onLoad = frameEvents.find(e => e.name === 'loadEventStart' && e.ts > navigationStart.ts);
+    const domContentLoaded = frameEvents.find(
+      e => e.name === 'domContentLoadedEventStart' && e.ts > navigationStart.ts


Actually......

The doc uses dclEnd rather than dclStart.
Since it's used as fi = Math.max(DCL, FI)... the End value seems much more accurate. (Handlers are setup during DCL, and the main thread is unavailable during this time)

paulirish · 2017-04-24T23:17:23Z

lighthouse-core/gather/computed/first-interactive.js

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


extract 50 to LONG_TASKS_THRESHOLD const

paulirish · 2017-04-24T23:21:14Z

lighthouse-core/gather/computed/first-interactive.js

+      throw new Error('trace not at least 5 seconds longer than FMP');
+    }
+
+    const longTasks = TracingProcessor.getMainThreadTopLevelEvents(traceModel, trace, FMP)


rename to longTasksAfterFMP

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

lighthouse-core/lib/traces/tracing-processor.js

+    const startEvent = trace.traceEvents.find(event => event.name === 'TracingStartedInPage');
+    const mainThread = TraceProcessor._findMainThreadFromIds(model, startEvent.pid, startEvent.tid);
+
+    // TODO(bckenny): filter for top level slices ourselves?


can we fix our longstanding #489 while you're here?

// See https://github.com/GoogleChrome/lighthouse/issues/489 const schedulableTaskTitle = 'TaskQueueManager::ProcessTaskFromWorkQueue'; const events = mainThread.sliceGroup.slices.filter(slice => { return slice.title === schedulableTaskTitle && slice.end > startTime && slice.start < endTime; });

Just tested this guy out and he's good. This also matches what we see in https://codereview.chromium.org/2610183003/patch/140001/150003

paulirish · 2017-04-24T23:50:11Z

lighthouse-core/test/lib/traces/pwmetrics-events-test.js

@@ -26,7 +26,7 @@ const dbwResults = require('../../fixtures/dbw_tester-perf-results.json');
 /* eslint-env mocha */
 describe('metrics events class', () => {
  it('exposes metric definitions', () => {
-    assert.equal(Metrics.metricsDefinitions.length, 12, '12 metrics not exposed');
+    assert.equal(Metrics.metricsDefinitions.length, 10, '10 metrics not exposed');


feel free to change this test. looks like it's getting annoying for you. :)

done, yes it was :)

paulirish · 2017-04-24T23:52:34Z

lighthouse-core/lib/traces/pwmetrics-events.js

@@ -131,39 +131,15 @@ class Metrics {
        }
      },
      {
-        name: 'Time to Interactive (vAlpha)',


let's keep TTI in here for now.
i'd like them to live side-by-side for a while for folks to get comfortable.
then we can drop.

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

lighthouse-core/gather/computed/first-interactive.js

+   * @return {number} The last index of the lonely task envelope,
+   *    -1 if not a lonely task
+   */
+  static getLastLonelyTaskIndex(longTasks, i, FMP, traceEnd) {


Over in #2023 you're using a candidate + while loop pattern that I find considerably easier to follow the logic of.

Is it possible to use the same pattern here?

take sorted list of tasks, lastLonelyCandidate is the first, while (lastLonelyCandidate.start < windowEnd)... kind of a thing..

for just getLastLonelyTaskIndex or the whole thing? The trickier piece here that makes everything annoying is the complicated base case differentiation. I planned to fix that by treating the entire span before FMP as one giant long task which will make the base case the same as considering the first long task, but not sure if I should do in this PR. If it's too hard to grok right now I can do that switch now instead of a follow-up, wdyt?

FWIW, I totally agree it's not nearly as easy to follow, but at least a tiny bit that's because lonely tasks are way harder to explain that simple quiet windows too :)

this is difficult to follow right now. "getLastLonelyTaskIndex" is kind of inscrutable...is it looking for the last one in the same envelope as the one at index i?

I planned to fix that by treating the entire span before FMP as one giant long task which will make the base case the same as considering the first long task

from this description, at least, this sounds like a generalization that will make the code harder to follow, but the proof is in the implementation :) If there are three(?) different zones (pre-FMP, FMP+5s, post-FMP+5s), what about explicitly handling them individually?

yeah I'll work on rewriting it

brendankenny · 2017-04-24T23:56:48Z

lighthouse-core/audits/first-interactive.js

+const SCORING_POINT_OF_DIMINISHING_RETURNS = 1700;
+const SCORING_MEDIAN = 10000;
+// This aligns with the external TTI targets in https://goo.gl/yXqxpL
+const SCORING_TARGET = 5000;


do we want to keep this at all? Meaning even less clear with the new curve :)

+1 I'll remove

brendankenny · 2017-04-24T23:57:57Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

+    const trace = artifacts.traces[Audit.DEFAULT_PASS];
+    return artifacts.requestFirstInteractive(trace).then(firstInteractive => {
+      const timeToFirstInteractive = firstInteractive.timeInMs;
+      const isFast = timeToFirstInteractive < MAXIMUM_TTI;


rename MAXIMUM_TTI (and other references to TTI in this file)?

brendankenny · 2017-04-25T00:02:57Z

lighthouse-core/gather/computed/first-interactive.js

+ * @see https://docs.google.com/document/d/1GGiI9-7KeY3TPqS3YT271upUVimo-XiL5mwWorDUD4c/edit#
+ *
+ * First Interactive marks the first moment when a website is minimally interactive:
+ *    > Enough (but maybe not all) UI components shown on the screen are interactive


is including this without a disclaimer misleading since this is the goal of the heuristic but is not actually tested here? If I were reading this I would be scanning the code for where event listeners are instrumented.

added disclaimer

brendankenny · 2017-04-25T00:05:38Z

lighthouse-core/gather/computed/first-interactive.js

+ * long tasks that are not "lonely".
+ *
+ *    > t = time in seconds since FMP
+ *    > N = 4 * e^(-0.045 * t) + 1


they're in the doc, but example values (and/or desmos curve) would be good here

brendankenny · 2017-04-25T00:13:11Z

lighthouse-core/gather/computed/first-interactive.js

+ *    > t = time in seconds since FMP
+ *    > N = 4 * e^(-0.045 * t) + 1
+ *    > a "lonely" task is an envelope of 250ms that contains a set of long tasks that have at least
+ *      1 second of padding before and after the envelope that contain no long tasks.


should include the FMP + 5s def?

brendankenny · 2017-04-25T00:21:52Z

lighthouse-core/lib/traces/tracing-processor.js

+   * @param {{traceEvents: !Array<!Object>}} trace
+   * @param {number=} startTime Optional start time (in ms) of range of interest. Defaults to trace start.
+   * @param {number=} endTime Optional end time (in ms) of range of interest. Defaults to trace end.
+   * @return {!Array<{start: number, end: number}>}


slice.duration is used in getMainThreadTopLevelEventDurations, not sure if there's anything else needed on these objects

added duration, or I could switch it to !Object if you prefer :)

brendankenny · 2017-04-25T00:27:29Z

lighthouse-core/lib/traces/tracing-processor.js

-        return;
-      }
-
+    slices.forEach(slice => {


you're harshing EQT perf here :S

oh puh-lease if we cared about perf here it wouldn't have taken months to realize the trace model was being constructed multiple times let alone that old TTI runs this forEach, filter, and sort about 100 times more than necessary :P

oh puh-lease if we cared about perf here it wouldn't have taken months to realize the trace model was being constructed multiple times let alone that TTI runs this forEach, filter, and sort about 100 times more than necessary :P

I didn't say anything about TTI :)

brendankenny · 2017-04-25T00:28:04Z

lighthouse-core/lib/traces/tracing-processor.js

+      return slice.end > startTime && slice.start < endTime;
+    });
+
+    events.sort((a, b) => a.start - b.start);


maybe at least sort in caller so getMainThreadTopLevelEventDurations doesn't sort twice?

brendankenny · 2017-04-25T00:33:29Z

lighthouse-core/lib/traces/tracing-processor.js

+   * @param {number=} endTime Optional end time (in ms) of range of interest. Defaults to trace end.
+   * @return {!Array<{start: number, end: number}>}
+   */
+  static getMainThreadTopLevelEvents(model, trace, startTime = 0, endTime = Infinity) {


can also use model.bounds.min and model.bounds.max for defaults here for slightly more self-evident values

hm, that actually feels a little less self-evident to me as someone who doesn't know the shape and meaning of the model properties well, we also wouldn't be able to specify them in the function signature anymore

hm, that actually feels a little less self-evident to me as someone who doesn't know the shape and meaning of the model properties well

-Infinity for startTime, then?

we also wouldn't be able to specify them in the function signature anymore

that would actually work. You can refer to earlier params in default parameters

-Infinity for startTime, then?

👍

that would actually work.

TIL

function noway({x, y} = {x: 3, y: 3}, woah = x) { console.log('boom', woah) } > noway() boom 3

brendankenny · 2017-04-25T00:44:30Z

lighthouse-core/gather/computed/first-interactive.js

+   * @return {number} The last index of the lonely task envelope,
+   *    -1 if not a lonely task
+   */
+  static getLastLonelyTaskIndex(longTasks, i, FMP, traceEnd) {


this is difficult to follow right now. "getLastLonelyTaskIndex" is kind of inscrutable...is it looking for the last one in the same envelope as the one at index i?

I planned to fix that by treating the entire span before FMP as one giant long task which will make the base case the same as considering the first long task

from this description, at least, this sounds like a generalization that will make the code harder to follow, but the proof is in the implementation :) If there are three(?) different zones (pre-FMP, FMP+5s, post-FMP+5s), what about explicitly handling them individually?

brendankenny · 2017-04-25T00:54:08Z

lighthouse-core/gather/computed/first-interactive.js

+ *    > N = 4 * e^(-0.045 * t) + 1
+ *    > a "lonely" task is an envelope of 250ms that contains a set of long tasks that have at least
+ *      1 second of padding before and after the envelope that contain no long tasks.
+ */


also document DOMContentLoaded bound?

patrickhulce · 2017-04-25T23:04:50Z

PTAL :)

paulirish

yeah this is sooooo much improved. thanks!

some naming bikeshedding but i'm otherwise happy

paulirish · 2017-04-26T00:11:42Z

lighthouse-core/gather/computed/first-interactive.js

+    let lastEndTime = -Infinity;
+    let currentGroup = null;
+
+    // consider all tasks that could possibly be part of a group starting before windowEnd


is the group starting before windowEnd or the tasks?
also here it's called considerationWindowEnd

we need to consider all tasks before considerationWindowEnd but we only want clusters that start before windowEnd

consider the case where window end is 15s, there's a 100ms task from 14.9-15s and a 500ms task from 15.5-16s, we need that later task to be clustered with the first so we can properly identify that main thread isn't quiet, but if that 100ms task weren't there we want to ignore it. Make sense? Not sure how to condense that explanation into a better comment though suggestions welcome :)

aside: would you consider considerationWindowEnd to be a clusteringWindowEnd ?

honestly i think you should just use the two sentences you just said ^ as the comment. :)

haha alright sg

paulirish · 2017-04-26T00:12:24Z

lighthouse-core/gather/computed/first-interactive.js

+
+    // consider all tasks that could possibly be part of a group starting before windowEnd
+    const considerationWindowEnd = windowEnd + MIN_TASK_GROUP_PADDING;
+    const couldTaskBeInGroup = task => task && task.start < considerationWindowEnd;


couldTaskBelongToGroup? unless tasks can be in multiple groups

paulirish · 2017-04-26T00:14:18Z

lighthouse-core/gather/computed/first-interactive.js

+   * @param {number} windowEnd
+   * @return {!Array<{start: number, end: number, duration: number}>}
+   */
+  static getTaskGroupsInWindow(tasks, startIndex, windowEnd) {


another naming bikeshed. wdyt about Cluster/TaskCluster rather than group? Implies the proximity that's part of this..

yeah I like cluster more, 👍

paulirish · 2017-04-26T00:15:16Z

lighthouse-core/gather/computed/first-interactive.js

+    for (let i = startIndex; couldTaskBeInGroup(tasks[i]); i++) {
+      const task = tasks[i];
+
+      // check if enough time has passed to start a new group


if enough time has elapsed, we'll create a new group

or cluster

paulirish · 2017-04-26T00:16:33Z

lighthouse-core/gather/computed/first-interactive.js

+
+      const isTooCloseToFMP = group => group.start < FMP + MIN_TASK_GROUP_FMP_DISTANCE;
+      const isTooLong = group => group.duration > MAX_TASK_GROUP_DURATION;
+      const isBadTaskGroup = group => isTooCloseToFMP(group) || isTooLong(group);


you could de-extract this guy. seems better to inline him in the .find() below.

I agree it's a bit much and I started with inlining when my names were shorter, but inlining now creates a linebreak and seems marginally better to keep the two conditions on the same line and keep each item a one-liner. still prefer a two-line inliner over this?

eh. not a huge deal to me.

paulirish · 2017-04-26T00:22:07Z

lighthouse-core/gather/computed/first-interactive.js

+  static getTaskGroupsInWindow(tasks, startIndex, windowEnd) {
+    const groups = [];
+
+    let lastEndTime = -Infinity;


previousTaskEndTime

patrickhulce · 2017-04-26T17:05:48Z

alright @brendankenny all you :)

patrickhulce · 2017-04-27T22:04:22Z

@brendankenny 🐜 ⛰ 🐒 🐈 🐩

brendankenny

Nice! Lots of comments from me, but all relatively superficial. This is looking just about ready to go

brendankenny · 2017-04-28T05:32:11Z

lighthouse-core/audits/first-interactive.js

+          score,
+          rawValue: firstInteractive.timeInMs,
+          displayValue: `${displayValue.toLocaleString()}ms`,
+          optimalValue: this.meta.optimalValue,


brendankenny · 2017-04-28T05:33:15Z

lighthouse-core/audits/load-fast-enough-for-pwa.js

 //   https://developers.google.com/web/progressive-web-apps/checklist
-const MAXIMUM_TTI = 10 * 1000;
+const MAXIMUM_TTFI = 10 * 1000;


I thought we were using TTCI here? Using FI as a threshold in offscreen-images.js is probably fine, but the banner isPWA check probably should use CI

I'll revert

nope I'm keeping again :)

nope I'm keeping again :)

I think @paulirish took the AI to get the message out about this for you :)

brendankenny · 2017-04-28T05:36:10Z

lighthouse-core/lib/traces/tracing-processor.js

+          slice.start < endTime;
+    });
+
+    events.sort((a, b) => a.start - b.start);


how do you feel about dropping sort and making callers do it?

not a fan, but I can live

i would prefer to do the sort as early as possible as we dont want audits to have to know things are unsorted

i'm decently confident that the sort is done by tracingmodel already, so TBH this line can probably just be nuked.

I'm adding tests with actual traces today now that it's in good shape so I'll find out and nuke the sort in caller if necessary

I'm adding tests with actual traces today now that it's in good shape so I'll find out and nuke the sort in caller if necessary

was this 👍 ?

yep pre-sorted by model!

brendankenny · 2017-04-28T05:41:56Z

lighthouse-core/audits/first-interactive.js

@@ -0,0 +1,73 @@
+/**
+ * @license
+ * Copyright 2017 Google Inc. All rights reserved.


you have confusing choices in license headers :)

it's whatever it was copied from :)

brendankenny · 2017-04-28T05:47:31Z

lighthouse-core/gather/computed/first-interactive.js

+ *
+ * If this timestamp is earlier than DOMContentLoaded, use DOMContentLoaded as firstInteractive.
+ */
+class FirstInteractive extends ComputedArtifact {


will you add to https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/closure/typedefs/ComputedArtifacts.js with /** @type {function(!Trace): !Promise<{timeInMs: number, timestamp: number}>} */ (I believe)

brendankenny · 2017-04-28T07:27:43Z

lighthouse-core/gather/computed/first-interactive.js

+    // task from 15.5-16s, we need that later task to be clustered with the first so we can properly
+    // identify that main thread isn't quiet.
+    const clusteringWindowEnd = windowEnd + MIN_TASK_CLUSTER_PADDING;
+    const couldTaskBelongToCluster = task => task && task.start < clusteringWindowEnd;


nit: maybe isInClusteringWindow? "could" is a low bar for descriptiveness :)

I've done zero performance testing, so I don't have a good basis for worrying, but if calculating FirstInteractive is (relatively) slow for some sites, doing out-of-bounds array accessing really isn't going to help things. What about something like an explicit regular i < tasks.length here and then do the if (!couldTaskBelongToCluster) break; (dropping the falsey task check) inside the loop?

brendankenny · 2017-04-28T07:37:50Z

lighthouse-core/gather/computed/first-interactive.js

+
+      // if enough time has elapsed, we'll create a new cluster
+      if (task.start - previousTaskEndTime > MIN_TASK_CLUSTER_PADDING) {
+        if (currentCluster) {


can you instead push upon creation of the cluster array (since it's known there's at least one task for the new cluster at this point) or is that going to miss one the other way?

brendankenny · 2017-04-28T07:38:51Z

lighthouse-core/gather/computed/first-interactive.js

+        return {start, end, duration, tasks};
+      })
+      // filter out clusters that started after the window because of our clusteringWindowEnd
+      .filter(cluster => cluster.start < windowEnd);


ah, nice 🎖

brendankenny · 2017-04-28T07:42:18Z

lighthouse-core/gather/computed/first-interactive.js

+      const task = tasks[i];
+
+      // if enough time has elapsed, we'll create a new cluster
+      if (task.start - previousTaskEndTime > MIN_TASK_CLUSTER_PADDING) {


I'm very curious why you went this way instead of clustering by 250ms and then doing a isNextClusterTooClose or whatever with 1000ms. Was there something easier this way or just felt more natural to you or...?

Not sure what you mean by clustering by 250ms since in this new way of viewing things that's more a condition on a task cluster rather than how a cluster is defined, what you're saying sounds mostly to me like why did I change it away from what I had before :)

Not sure what you mean by clustering by 250ms since in this new way of viewing things that's more a condition on a task cluster rather than how a cluster is defined, what you're saying sounds mostly to me like why did I change it away from what I had before :)

yeah, I was curious about your reason for switching

oh this just seemed more straightforward after talking it out with paul, easier to explain exactly what's happening without looking at code

brendankenny · 2017-04-28T07:55:13Z

lighthouse-core/gather/computed/first-interactive.js

+        const start = tasks[0].start;
+        const end = tasks[tasks.length - 1].end;
+        const duration = end - start;
+        return {start, end, duration, tasks};


any reason to keep tasks around? (not on the return type and looks like not used, so OK to drop?)

was just for debugging, can drop

brendankenny · 2017-04-28T08:32:24Z

I think there is one case this is missing: once a task is selected as the start of a window, getTaskClustersInWindow doesn't consider if the next tasks examined should have been clustered with that first task instead of just with each other.

So, e.g. (in test form)

mainThreadEvents = [
  {start: 5500, end: 5600},
  {start: 6000, end: 6100},
];

const result = firstInteractive.computeWithArtifacts({}, {}, {
  timings: {
    firstMeaningfulPaint: 1000,
    domContentLoaded: 800,
    traceEnd: 24000,
  },
  timestamps: {
    navigationStart: 0,
  },
});

says that FirstInteractive is at 5600, even though that's less than 1000ms away from the next long task, so it should be 6100 (correct me if the example is wrong; it's late :)

I believe the fix could be as simple as adding a

if (i + 1 < longTasks.length &&
    longTasks[i + 1].start - windowStart <= MIN_TASK_CLUSTER_PADDING) {
  continue;
}

after the windowEnd > traceEnd check in findQuietWindow, but there might be a better/prettier way of doing it

patrickhulce · 2017-04-28T18:00:08Z

says that FirstInteractive is at 5600, even though that's less than 1000ms away from the next long task, so it should be 6100 (correct me if the example is wrong; it's late :)

yup good call, didn't have a test for this before and got missed in refactor

patrickhulce · 2017-04-28T21:06:17Z

alright I believe all feedback addressed, added a couple tests for called out cases and the ones @brendankenny pointed out, added a new trace with 5s quiet on pwa.rocks to assert TTFI is correct with actual trace

brendankenny

This is a lovely implementation.

Three tiny (and optional) nits, but otherwise looks great to me

brendankenny · 2017-04-29T01:41:19Z

lighthouse-core/gather/computed/first-interactive.js

+    }
+
+    if (!FMP || !DCL) {
+      throw new Error(`No ${FMP ? 'domContentLoaded' : 'firstMeaningfulPaint'} event in trace`);


should we throw in traceOfTab just to ruin all dependent audits' days?

I only moved FMP since DCL is specific to TTFI but everything needs FMP basically

well, nevermind we explicitly test that trace-of-tab can handle those traces without paints...reverting for now and we can do in a followup PR if necessary

reverting for now and we can do in a followup PR if necessary

I guess having #1981 open will have to suffice for now :(

brendankenny · 2017-04-29T01:41:38Z

lighthouse-core/gather/computed/first-interactive.js

+    // task from 15.5-16s, we need that later task to be clustered with the first so we can properly
+    // identify that main thread isn't quiet.
+    const clusteringWindowEnd = windowEnd + MIN_TASK_CLUSTER_PADDING;
+    const isInClusteringWindow = task => task && task.start < clusteringWindowEnd;


nit: task will always be defined here now

brendankenny · 2017-04-29T01:54:35Z

lighthouse-core/test/gather/computed/first-interactive-test.js

+  });
+
+  it('throws on short traces', () => {
+    return firstInteractive.compute_({traceEvents: tooShortTrace}, computedArtifacts).then(() => {


nit: for these, can this call computedArtifacts.requestFirstInteractive({traceEvents: tooShortTrace}) so it doesn't have to call the underlying compute_?

brendankenny · 2017-04-29T01:58:53Z

oh, sorry for the conflict :S

patrickhulce · 2017-04-29T05:57:28Z

oh, sorry for the conflict :S

geez even after approval you're givin' me a hard time ;)

brendankenny · 2017-04-30T23:36:58Z

⏱🖌🏁⏱

benschwarz · 2017-05-01T07:14:57Z

👏 Great work @patrickhulce && @brendankenny

feat: add firstInteractive

940432a

patrickhulce commented Apr 14, 2017

View reviewed changes

typo in test

bb216cb

paulirish reviewed Apr 15, 2017

View reviewed changes

brendankenny mentioned this pull request Apr 15, 2017

allow computed artifacts to request other computed artifacts #2018

Merged

patrickhulce added 4 commits April 17, 2017 09:06

feedback

1aaf58a

fix smoke tests

145cb90

Merge branch 'master' into first_interactive

a927687

fix merge conflicts

539f1ac

swanky new scoring graph

dcc22cc

paulirish requested changes Apr 24, 2017

View reviewed changes

paulirish reviewed Apr 24, 2017

View reviewed changes

paulirish reviewed Apr 25, 2017

View reviewed changes

brendankenny requested changes Apr 25, 2017

View reviewed changes

brendankenny reviewed Apr 25, 2017

View reviewed changes

feedback

7729fb1

patrickhulce force-pushed the first_interactive branch from ab7e6fb to 7729fb1 Compare April 25, 2017 16:34

patrickhulce mentioned this pull request Apr 25, 2017

feat: add consistently interactive audit #2023

Merged

patrickhulce added 4 commits April 25, 2017 10:44

Merge branch 'master' into first_interactive

dde547f

when you're right you're right

3a9f25e

easier to understand TTFI

6059f91

switch fileoverview wording to match impl

d8c4a5a

paulirish approved these changes Apr 26, 2017

View reviewed changes

patrickhulce added 2 commits April 26, 2017 08:57

rename group -> cluster

ce56b37

updated comments

41524bd

paulirish assigned brendankenny Apr 27, 2017

brendankenny requested changes Apr 28, 2017

View reviewed changes

patrickhulce added 2 commits April 28, 2017 13:57

feedback

4f90902

Merge branch 'master' into first_interactive

d1b0f60

brendankenny approved these changes Apr 29, 2017

View reviewed changes

patrickhulce added 2 commits April 28, 2017 22:58

Merge branch 'master' into first_interactive

2e74f05

nits

008692d

patrickhulce force-pushed the first_interactive branch from 032bc61 to 008692d Compare April 29, 2017 06:08

brendankenny merged commit da8e097 into master Apr 30, 2017

brendankenny deleted the first_interactive branch April 30, 2017 23:37

patrickhulce mentioned this pull request May 1, 2017

Properly account for non-top-level toplevel tasks for estimating input latency #489

Closed

paulirish pushed a commit to paulirish/lighthouse that referenced this pull request May 5, 2017

feat: add firstInteractive (GoogleChrome#2013)

773d90b

feat: add firstInteractive #2013

feat: add firstInteractive #2013

Conversation

patrickhulce commented Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

paulirish Apr 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

brendankenny Apr 15, 2017 • edited Loading

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 Apr 15, 2017 via email

brendankenny commented Apr 17, 2017

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

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

patrickhulce Apr 25, 2017 • edited Loading

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 Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 25, 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

paulirish Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 14, 2017 •

edited

Loading

paulirish Apr 15, 2017 •

edited

Loading

brendankenny Apr 15, 2017 •

edited

Loading

patrickhulce Apr 25, 2017 •

edited

Loading

brendankenny Apr 25, 2017 •

edited

Loading

paulirish Apr 26, 2017 •

edited

Loading