Introduce TraceOfTab computed artifact #1549

paulirish · 2017-01-27T02:51:06Z

We often need to parse a trace and find the main thread, its navigationStart, and get our thread events available and sorted.

This PR introduces a computed artifact that can be reused across audits. It also migrates FMP, TTI, and user-timing audits to leverage it.

Reviewers: there is some noisiness in the diff that ?w=1 will nuke.

--

should fix #762

…ne-model.

patrickhulce · 2017-01-27T20:07:26Z

lighthouse-core/audits/first-meaningful-paint.js

    // fMP will follow at/after the FCP, though we allow some timestamp tolerance
-    const firstMeaningfulPaint = frameEvents.find(e =>
+    const firstMeaningfulPaint = tabTrace.traceEvents.find(e =>


I know this is an FMP audit but seems a little strange that it's missing from the TraceOfTab while FCP is still there. Does FCP never fire twice but FMP does and that's why we have to worry about it here?

FCP is super reliable in a trace. I use FCP to select the best navStart and so far that is the most robust method. (if this werent the case, i wouldnt have grabbed FCP there)

FMP is definitely not (and im thinking we should soon write our own FMP selection based on the candidates, as the real one's network idle detection is taking too long. anyway..)

however technically i could do this FMP selection in the computed artifact just so that TTI audit doesn't need to depend on both of the taboftrace artifact and fmp audit at the same time. that's fine with me.

Moving any proper event deduping/finding to one place sounds good to me!

patrickhulce · 2017-01-27T20:09:31Z

lighthouse-core/audits/user-timings.js

+
+    // Get all blink.user_timing events
+    // The event phases we are interested in are mark and instant events (R, i, I)
+    // and duration events which correspond to measures (B, b, E, e).


Wow these are really fun names :)

patrickhulce · 2017-01-27T20:09:47Z

lighthouse-core/audits/user-timings.js

+      // Mark events fall under phases R and I (or i)
+      if (ut.ph === 'R' || ut.ph.toUpperCase() === 'I') {
+        // Add user timings event to array
+        if (ut.name !== 'navigationStart') {


Can't we move this above and simplify a bit?

yah definitely. I'll put this in the earlier rejection filter. good call.

brendankenny · 2017-01-27T20:52:53Z

love this

paulirish · 2017-01-27T22:52:58Z

updated. and tests added. ptal

brendankenny

this is going to be great

brendankenny · 2017-01-27T23:09:16Z

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

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


brendankenny · 2017-01-27T23:09:36Z

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

+const ComputedArtifact = require('./computed-artifact');
+const fMPAudit = require('../../audits/first-meaningful-paint');
+
+class TraceOfTab extends ComputedArtifact {


will you add a fileoverview or class-level doc on what this provides?

brendankenny · 2017-01-27T23:59:18Z

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

+    const frameEvents = keyEvents.filter(e => e.args.frame === startedInPageEvt.args.data.page);
+
+    // Find our first FCP
+    const firstFCP = frameEvents.find(e => e.name === 'firstContentfulPaint');


so a common cause of current audit failures is not finding the FCP event and throwing on firstFCP.ts, below. Trying it with airhorner now, I regularly get traces with no firstContentfulPaint anywhere in the trace. Is there a backup method for identifying navStart in that instance and a way to handle other calculations based on FCP? Maybe if there's only a single one of them? Or is it possible there's only one but it's still wrong?

for reference, in an example trace, here are the names of the events in frameEvents at this point:
['navigationStart', 'fetchStart', 'responseEnd', 'unloadEventStart', 'unloadEventEnd', 'domLoading', 'firstLayout', 'firstPaint', 'domInteractive', 'domContentLoadedEventStart', 'domContentLoadedEventEnd', 'domComplete', 'loadEventStart', 'loadEventEnd']

sounds good. mind if i fix that bug in a followup?
happy to, just want to add proper tests for it

mind if i fix that bug in a followup?

yeah, sounds good

filed #1567

brendankenny · 2017-01-28T00:17:14Z

lighthouse-core/audits/first-meaningful-paint.js

+  // being up to 1ms BEFORE the fCP. We're okay if this happens, however if we see
+  // a gap of more than 2 frames (32,000 microseconds), then it's a bug that should
+  // be addressed in FirstMeaningfulPaintDetector.cpp
+  static get toleranceMs() {


this should live in trace-of-tab.js now

brendankenny · 2017-01-28T00:21:46Z

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

@@ -0,0 +1,53 @@
+/**
+ * Copyright 2016 Google Inc. All rights reserved.


brendankenny · 2017-01-28T00:24:10Z

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

+    }).sort((event0, event1) => event0.ts - event1.ts);
+
+    return {
+      traceEvents: allFrameEvents,


maybe something in the property name that these are specifically from the one process?

processEvents ? I kind of like having that part kind of transparent, TBH. So i'd rather keep it the standard name for the array, but let me know if you feel strongly

yeah, I get the transparency argument too, but there's also the case of -- I'm writing a gatherer that needs trace events, how do I figure out which events belong to my process? Unless you read the source of this you wouldn't know. Maybe fixable with just some docs, though :)

renamed to processEvents. :)

brendankenny · 2017-01-28T00:26:20Z

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

+/* eslint-env mocha */
+describe('Trace of Tab computed artifact:', () => {
+  describe('finds correct FMP in various traces', () => {
+    it('finds the fMP if there was a tracingStartedInPage after the frame\'s navStart', () => {


any other invariants to check? Maybe they all have the same pid?

brendankenny · 2017-01-28T00:27:47Z

lighthouse-core/audits/first-meaningful-paint.js

-    // fMP will follow at/after the FCP, though we allow some timestamp tolerance
-    const firstMeaningfulPaint = frameEvents.find(e =>
-        e.name === 'firstMeaningfulPaint' && e.ts >= (firstFCP.ts - FCPFMP_TOLERANCE));
+  static collectEvents(tabTrace) {


having a function for this is kind of overkill now. Inline in either audit or calculateScore, maybe?

true! but #1564 will make it more valuable.

paulirish · 2017-01-28T02:19:52Z

ptal

brendankenny

LGTM! ✏️ 📑

paulirish added 3 commits January 26, 2017 08:20

Introduce TraceOfTab computed artifact, and migrate FMP audit to it.

9764d94

Migrate uesr-timing to trace-of-tab artifact, and off devtools-timeli…

8ed1940

…ne-model.

update tti test

a6fbab4

patrickhulce reviewed Jan 27, 2017

View reviewed changes

paulirish added 2 commits January 27, 2017 14:50

feedback

63b3c5c

Add dedicated tests for trace-of-tab

2645fcf

brendankenny requested changes Jan 28, 2017

View reviewed changes

paulirish added 2 commits January 27, 2017 17:58

feedback.

6d1ee89

rename to processEvents. nuke fmp.collectEvents

7c72fb2

paulirish mentioned this pull request Jan 28, 2017

Support traces where a FCP isn't found #1567

Closed

brendankenny approved these changes Jan 28, 2017

View reviewed changes

brendankenny merged commit 7a3dbbd into master Jan 28, 2017

brendankenny deleted the trace-of-tab branch January 28, 2017 02:35

brendankenny mentioned this pull request Mar 14, 2017

use FMP from TraceOfTab in TTI calc #1858

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce TraceOfTab computed artifact #1549

Introduce TraceOfTab computed artifact #1549

paulirish commented Jan 27, 2017 •

edited

Loading

patrickhulce Jan 27, 2017

paulirish Jan 27, 2017

patrickhulce Jan 27, 2017

patrickhulce Jan 27, 2017

patrickhulce Jan 27, 2017

paulirish Jan 27, 2017

brendankenny commented Jan 27, 2017

paulirish commented Jan 27, 2017

brendankenny left a comment

brendankenny Jan 27, 2017

brendankenny Jan 27, 2017

brendankenny Jan 27, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

brendankenny Jan 28, 2017

paulirish Jan 28, 2017

paulirish Jan 28, 2017

paulirish commented Jan 28, 2017

brendankenny left a comment

		@@ -0,0 +1,53 @@
		/**
		* Copyright 2016 Google Inc. All rights reserved.

Introduce TraceOfTab computed artifact #1549

Introduce TraceOfTab computed artifact #1549

Conversation

paulirish commented Jan 27, 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

brendankenny commented Jan 27, 2017

paulirish commented Jan 27, 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

paulirish commented Jan 28, 2017

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Jan 27, 2017 •

edited

Loading