core: support traces with TracingStartedInBrowser event #5271

a1ph · 2018-05-19T01:15:29Z

Since version 67 Chrome uses a new schema to describe page frame tree.
The tree is encoded within TracingStartedInBrowser trace event.
The patch makes lighthouse to support the new event.
Legacy TracingStartedInPage event is going to the removed soon.

brendankenny

thanks for the PR!

brendankenny · 2018-05-21T19:34:46Z

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

@@ -45,18 +45,30 @@ class TraceOfTab extends ComputedArtifact {
      .filter(e => {
        return e.cat.includes('blink.user_timing') ||
          e.cat.includes('loading') ||
-          e.cat.includes('devtools.timeline') ||
-          e.name === 'TracingStartedInPage';


think you don't want to filter this out to support the legacy case? Or in the fallback case need to search for the first TracingStartedInPage in the original trace.traceEvents

also, does the filter now need to let through e.name === 'TracingStartedInBrowser' events?

Both TracingStartedInPage and TracingStartedInBrowser are in devtools.timeline category which is already allowed.

It seems like it was in toplevel once upon a time based on the fixture change below?

my traces from ~December have it in disabled-by-default-devtools.timeline so I guess it's a question of how far back we want to go

AFAICT TracingStartedInPage was introduced 4 years ago with devtools.timeline category. See https://codereview.chromium.org/254613002/diff/90001/Source/core/inspector/InspectorTracingAgent.cpp
I wonder where the traces with it being in toplevel come from?

Hm, no clue then 🤷‍♂️ 4 years is plenty good enough for me 😆

brendankenny · 2018-05-21T19:35:23Z

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

+    // Find out the inspected page frame.
+    let startedInPageEvt;
+    const startedInBrowserEvt = keyEvents.find(e => e.name === 'TracingStartedInBrowser');
+    if (startedInBrowserEvt && startedInBrowserEvt.args.data.persistentIds) {


what's the significance of persistentIds? doesn't appear to be used anywhere?

We used to have a TracingStartedInBrowser event before, but with no frames information. persistentIds is a marker that frames are provided.

brendankenny · 2018-05-21T19:39:34Z

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

+    const startedInBrowserEvt = keyEvents.find(e => e.name === 'TracingStartedInBrowser');
+    if (startedInBrowserEvt && startedInBrowserEvt.args.data.persistentIds) {
+      const mainFrame = (startedInBrowserEvt.args.data.frames || []).find(frame => !frame.parent);
+      const mainFrameId = mainFrame && mainFrame.frame || '';


what's the use of the fallback '' value here if there's no mainFrame or mainFrameId found? Seems like we need those and should just throw if not found

(or could set startedInPageEvt to undefined and let the legacy fallback below try to find a backup TracingStartedInPage event)

brendankenny · 2018-05-21T19:42:10Z

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

+      // Beware: the tracingStartedInPage event can appear slightly after a navigationStart
+      startedInPageEvt = keyEvents.find(e => e.name === 'TracingStartedInPage');
+      if (!startedInPageEvt) throw new LHError(LHError.errors.NO_TRACING_STARTED);
+      // @ts-ignore - property chain exists for 'TracingStartedInPage' event.


two lines above should be pulled out of the conditional (throw if no startedInPageEvt found, // @ts-ignore comment is for the const frameId = line below)

It doesn't really matter as when we didn't enter the if block the startedInPageEvt is already there. Anyway, I will move it.

patrickhulce · 2018-05-23T23:30:05Z

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

@@ -16,6 +16,8 @@ const preactTrace = require('../../fixtures/traces/preactjs.com_ts_of_undefined.
 const noFMPtrace = require('../../fixtures/traces/no_fmp_event.json');
 const noFCPtrace = require('../../fixtures/traces/airhorner_no_fcp');
 const backgroundTabTrace = require('../../fixtures/traces/backgrounded-tab-missing-paints');
+const tracingStartedInBrowserTrace =
+  require('../../fixtures/traces/tracing-started-in-browser.json');


this whole fixture is just to exercise that one event right?

could we use a reduced trace or even just a fake array of events for this? 6k lines for the 1 relevant event is a lot :)

patrickhulce · 2018-05-23T23:33:20Z

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

@@ -45,18 +45,30 @@ class TraceOfTab extends ComputedArtifact {
      .filter(e => {
        return e.cat.includes('blink.user_timing') ||
          e.cat.includes('loading') ||
-          e.cat.includes('devtools.timeline') ||
-          e.name === 'TracingStartedInPage';


It seems like it was in toplevel once upon a time based on the fixture change below?

my traces from ~December have it in disabled-by-default-devtools.timeline so I guess it's a question of how far back we want to go

patrickhulce · 2018-05-23T23:34:55Z

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

@@ -106,12 +121,13 @@ class TraceOfTab extends ComputedArtifact {
    // stable-sort events to keep them correctly nested.
    /** @type Array<LH.TraceEvent> */
    const processEvents = trace.traceEvents
+      // @ts-ignore - property chain exists for 'TracingStartedInPage' event.


hmmm, we shouldn't need the ts-ignore for .pid, what was the error you're suppressing?

error TS2532: Object is possibly 'undefined'.

Oh because startedInPageEvt is LH.TraceEvent|undefined and we're accessing inside a closure :/

seems like we just need progress on microsoft/TypeScript#11498, mind replacing the ts-ignore explanation with that link or casting instead? /** @type {LH.TraceEvent} */ (startedInPageEvt).pid (same for .tid below)

Sure, I can annotate. I surprised the compiler hasn't noticed if (!startedInPageEvt) throw

It knows, it's just that there's no mechanism for telling if the callback will be invoked asynchronously or just once synchronously.

const filtered = [] for (const e of events) if (e.pid === startedInPageEvt.pid) filtered.push(e)

works fine

yeah, they still do a lot simpler CFG work than closure, focusing almost entirely locally, so things like this still trip them up even while being more sophisticated in other ways :)

brendankenny · 2018-05-24T18:33:16Z

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

+      // The first TracingStartedInPage in the trace is definitely our renderer thread of interest
+      // Beware: the tracingStartedInPage event can appear slightly after a navigationStart
+      startedInPageEvt = keyEvents.find(e => e.name === 'TracingStartedInPage');
+    }
    if (!startedInPageEvt) throw new LHError(LHError.errors.NO_TRACING_STARTED);
    // @ts-ignore - property chain exists for 'TracingStartedInPage' event.
    const frameId = startedInPageEvt.args.data.page;


if the casts are annoying, could also do something like

/** @type {LH.TraceEvent|undefined} */ let possibleStartedInPageEvt; ...

above, and then here

const startedInPageEvt = possibleStartedInPageEvt;

because the compiler will know that possibleStartedInPageEvt is definitely defined at this point, the type for startedInPageEvt will be inferred correctly as LH.TraceEvent, and then it won't need any casts below this.

I considered that, but I didn't want to introduce more code to workaround compiler issues. I'm fine with casts.

patrickhulce

LGTM! thanks for helping us out here @a1ph! 👍

patrickhulce · 2018-05-25T16:56:47Z

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

+        'args': {'name': 'CrRendererMain'},
+      }]};
+    const trace = await traceOfTab.compute_(tracingStartedInBrowserTrace);
+    assert.equal(trace.startedInPageEvt.ts, 2193564729582);


want to assert the navStart is found and in the trace.mainThreadEvents since I assume that's what the thread_name addition was for?

Since version 67 Chrome uses a new schema to describe page frame tree. The tree is encoded within TracingStartedInBrowser trace event. The patch makes lighthouse to support the new event. Legacy TracingStartedInPage event is going to the removed soon.

patrickhulce · 2018-05-31T20:00:16Z

@brendankenny does this look good to go to you?

brendankenny

just a few style nits, but otherwise LGTM! Thanks for making the change

brendankenny · 2018-06-01T17:26:04Z

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

-      .filter(e => {
-        return e.cat.includes('blink.user_timing') ||
+      .filter(e =>
+          e.cat.includes('blink.user_timing') ||


generally for multi-line arrow function we keep the curly braces/return for clarity. Any reason to drop it here?

brendankenny · 2018-06-01T17:26:21Z

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

+    let startedInPageEvt;
+    const startedInBrowserEvt = keyEvents.find(e => e.name === 'TracingStartedInBrowser');
+    if (startedInBrowserEvt && startedInBrowserEvt.args.data &&
+        startedInBrowserEvt.args.data.persistentIds) {


can you add a comment about why persistentIds is significant?

I talked to dgozman, it was added to ensure this is a new version of the event that carries frames. I think we can just check for frames field being there.

brendankenny · 2018-06-01T17:28:10Z

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

+      const threadNameEvt = keyEvents.find(e => e.pid === pid && e.ph === 'M' &&
+        e.cat === '__metadata' && e.name === 'thread_name' &&
+        // @ts-ignore - property chain exists for 'thread_name' event.
+        e.args.name === 'CrRendererMain');


since it's just a single level into args, add name?: string to args in externs.d.ts instead of @ts-ignore here?

patrickhulce · 2018-06-04T20:19:40Z

oops you got some conflicts now @a1ph! I had fixed the feedback before you pushed sorry!

Since version 67 Chrome uses a new schema to describe page frame tree. The tree is encoded within TracingStartedInBrowser trace event. The patch makes lighthouse to support the new event. Legacy TracingStartedInPage event is going to the removed soon.

patrickhulce · 2018-06-04T22:23:15Z

thanks @a1ph!

a1ph requested review from patrickhulce and paulirish as code owners May 19, 2018 01:15

paulirish deployed to PR staging May 19, 2018 01:15 View deployment

paulirish mentioned this pull request May 19, 2018

misc(scripts): add trace/devtoolslog minification scripts #5237

Merged

brendankenny requested changes May 21, 2018

View reviewed changes

a1ph changed the title ~~core: Support traces with TracingStartedInBrowser event~~ core: support traces with TracingStartedInBrowser event May 23, 2018

a1ph force-pushed the master branch 2 times, most recently from 6840233 to 678d4aa Compare May 23, 2018 23:27

patrickhulce reviewed May 23, 2018

View reviewed changes

a1ph force-pushed the master branch 2 times, most recently from 26c4539 to 9f252e3 Compare May 24, 2018 18:19

brendankenny reviewed May 24, 2018

View reviewed changes

a1ph force-pushed the master branch from 9f252e3 to 9249083 Compare May 24, 2018 19:12

patrickhulce approved these changes May 25, 2018

View reviewed changes

a1ph force-pushed the master branch from 9249083 to 9785181 Compare May 26, 2018 01:10

brendankenny approved these changes Jun 1, 2018

View reviewed changes

patrickhulce added 3 commits June 4, 2018 10:50

add name to externs

328cd03

brendan's comments

3678880

semicolon

d63f491

a1ph force-pushed the master branch from 1b3076a to 5b1793d Compare June 4, 2018 20:16

a1ph force-pushed the master branch from 5b1793d to 559ae44 Compare June 4, 2018 20:26

patrickhulce merged commit df631a0 into GoogleChrome:master Jun 4, 2018

core: support traces with TracingStartedInBrowser event #5271

core: support traces with TracingStartedInBrowser event #5271

Conversation

a1ph commented May 19, 2018

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

patrickhulce May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce May 24, 2018 • 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

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented May 31, 2018

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

patrickhulce commented Jun 4, 2018

patrickhulce commented Jun 4, 2018

patrickhulce May 24, 2018 •

edited

Loading

patrickhulce May 24, 2018 •

edited

Loading