core: remove dependency on DevtoolsTimelineModel #5533

patrickhulce · 2018-06-20T23:16:17Z

this was actually quite a bit smoother than network records thanks to how little we used it, I've now confirmed that the numbers reported in the mainthread-work-breakdown tests are actually what show up in DevTools for the same trace 🎉

(we break scripting out into 3 chunks but 215+25+48=288)

taking CNN as an example, we were previously missing ~16s of main thread time, ~5s of JS bootup time. it previously took ~2s to compute the devtools timeline model, we now do the computations we need (twice) in ~200ms.

brendankenny · 2018-06-22T20:26:47Z

lighthouse-core/audits/bootup-time.js

+      if (!task.attributableURL || task.attributableURL === 'about:blank') continue;
+
+      const timingByGroupId = result.get(task.attributableURL) || {};
+      const original = timingByGroupId[task.group.id] || 0;


bikeshed: originalTime?

brendankenny · 2018-06-22T20:29:29Z

lighthouse-core/audits/mainthread-work-breakdown.js

-      result.set(eventName, event.selfTime));
+
+    for (const task of tasks) {
+      result.set(task.group.id, (result.get(task.group.id) || 0) + task.selfTime * multiplier);


split second part into a variable(s)?

did the first part

brendankenny · 2018-06-22T20:33:20Z

lighthouse-core/lib/task-groups.js

-  taskToGroup,
+  taskGroups,
+  taskNameToGroup,
+  traceEventNames,


traceEventNames unused?

yeah I was originally using it to limit the events I had to process in hierarchy but it wasn't a huge perf win, removed

brendankenny · 2018-06-22T20:34:32Z

lighthouse-core/lib/task-groups.js

+ * @see https://cs.chromium.org/chromium/src/third_party/blink/renderer/devtools/front_end/timeline/TimelineUIUtils.js?type=cs&q=_initEventStyles+-f:out+f:devtools&sq=package:chromium&g=0&l=39
+ */
+const taskGroups = {
+  ParseHTML: {


Any reason to capitalize first letter?

:) but not really, matching DevTools isn't worth much in a removal of DT deps...

brendankenny · 2018-06-22T20:36:45Z

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


 class TraceProcessor {
+  /**
+   * @param {LH.TraceEvent} event


we may need some docs here. What is a task and why does it matter vs an event? Should this just be its own class/file?

brendankenny · 2018-06-22T20:40:50Z

lighthouse-core/audits/bootup-time.js

-        text: groupIdToName.scriptParseCompile},
-    ];
-
+    const tasks = TraceProcessor.getMainThreadTasks(trace.traceEvents);


should this be a computed artifact?

yeah probably, ☠️ 2️⃣ 🐦 1️⃣ 🗿

brendankenny · 2018-06-22T21:59:14Z

lighthouse-core/gather/computed/screenshots.js

+      .map(evt => {
+        return {
+          timestamp: evt.ts / 1000,
+          datauri: `data:image/jpg;base64,${evt.args.snapshot}`,


I'm surprised the compiler doesn't object to accessing snapshot

well I added snapshot to traceevent definition :)

I meant because it was optional but looks like the compiler's ok passing undefined into template strings :)

patrickhulce · 2018-06-26T18:39:37Z

FYI This is the only one open I'd really want to make sure lands in 3.0 this week @paulirish @brendankenny so we can jump on 3rd party analysis :)

paulirish

Nice job on the main-thread-tasks library. It's hot. There's gonna be demand to ship it standalone. ;)

I think you can remove the console-quieter file now, too. Woo!

Some questions and comments below..

paulirish · 2018-06-26T19:19:44Z

lighthouse-core/lib/task-groups.js

-  'XHR Load': groupIdToName.scripting,
-  'XHR Ready State Change': groupIdToName.scripting,
+/**
+ * Make sure the traceEventNames keep up with the ones in DevTools


curious.. did you make any updates for new names this time?

i reviewed https://chromium.googlesource.com/chromium/src/+blame/566d3c11bcb0059ec695185808d33bb4474fb4d7/third_party/blink/renderer/devtools/front_end/timeline_model/TimelineModel.js#1156 and only saw one obvious omission aside from some odd crypto ones.

Well the whole list is new :) last time we were keying off the display names, I pulled everything I saw that I recognized/looked like it could be substantial

paulirish · 2018-06-26T19:42:24Z

lighthouse-core/lib/task-groups.js

+      'InvalidateLayout',
+      'Layout',
+      'UpdateLayer',
+      'UpdateLayerTree',


UpdateLayer & UpdateLayerTree belong in paintCompositeRender

whoops, done

paulirish · 2018-06-26T19:50:55Z

lighthouse-core/test/results/sample_v2.json

-            "duration": 63.04200002551079
+            "group": "other",
+            "groupLabel": "Other",
+            "duration": 177.20099999999977


FWIW i reviewed these totals and compared them against what perf panel reports.
they lgtm!

paulirish · 2018-06-26T19:57:14Z

lighthouse-core/audits/bootup-time.js

+
+      const timingByGroupId = result.get(task.attributableURL) || {};
+      const originalTime = timingByGroupId[task.group.id] || 0;
+      timingByGroupId[task.group.id] = originalTime + task.selfTime * multiplier;


feels weird to apply the multiplier in here.
i know it's another loop, but i'd rather apply the multiplier after we loop over all the tasks.

Oh funny I moved it there because it natural in the computation of time instead of a post-step :)

I don't feel that strongly though, done.

ha, it reads better inlined to me too

paulirish · 2018-06-26T19:57:30Z

lighthouse-core/audits/mainthread-work-breakdown.js

+
+    for (const task of tasks) {
+      const originalTime = result.get(task.group.id) || 0;
+      result.set(task.group.id, originalTime + task.selfTime * multiplier);


same comment about the inlined multiplier.

paulirish · 2018-06-26T20:18:23Z

lighthouse-core/gather/computed/main-thread-tasks.js

+      MainThreadTasks._computeRecursiveTaskGroup(task);
+    }
+
+    const firstTs = (tasks[0] || {startTime: 0}).startTime;


i've always heard the traceEvents could come out of order, however I know this is more of a case of cross-process events. Perhaps not an issue if everything is within the same thread? We should doublecheck with caseq.

oh yeah, we should make sure to use the sorted events here 👍

oh yeah, we should make sure to use the sorted events here 👍

should this depend on traceOfTab.mainThreadEvents? Then it doesn't need to check if tasks are in the main thread and they're already sorted

yup that was my plan 👍

paulirish · 2018-06-26T20:19:19Z

lighthouse-core/gather/computed/main-thread-tasks.js

+    }
+
+    const firstTs = (tasks[0] || {startTime: 0}).startTime;
+    for (const task of tasks) {


add comment

Baselining all times to be relative to start of the trace, and in milliseconds

paulirish · 2018-06-26T20:22:39Z

lighthouse-core/lib/task-groups.js

+      'UpdateLayerTree',
+    ],
+  },
+  paintCompositeRender: {


I noticed you merged together painting & compositing. I'll survive. 😣

yeah trying to reduce noise for newbs, if you're digging into the savings/differences there it's time for perf panel/chrome tracing 😛

WFM. plus, i think this distinction matters more in the 60fps/jank case than pageload.

paulirish · 2018-06-26T20:26:47Z

lighthouse-core/lib/task-groups.js

+      'FireIdleCallback',
+      'FireAnimationFrame',
+      'RunMicrotasks',
+      'V8.Execute',


add 'v8.evaluateModule'

paulirish · 2018-06-26T20:29:35Z

lighthouse-core/audits/bootup-time.js

-          groups[name] = value * multiplier;
-          totalBootupTime += value * multiplier;
+        let bootupTimeForURL = 0;
+        for (const timespanMs of Object.values(timingByGroupId)) {


not since we're moving the multiplier down, otherwise yes :)

brendankenny · 2018-06-26T20:17:40Z

lighthouse-core/audits/bootup-time.js

-   * @return {Map<string, Object<string, number>>}
+   * @param {LH.Artifacts.TaskNode[]} tasks
+   * @param {number} multiplier
+   * @return {Map<string, Object<keyof taskGroups, number>>}


if you use keyof in the Object<> type (or anything reasonably complicated, it seems), tsc gets confused and promotes to any, so should use Record<> here (maybe we should just give up on Object and always use Record)

had to widen it to string anyhow

brendankenny · 2018-06-26T21:18:19Z

lighthouse-core/audits/bootup-time.js

+
+      const timingByGroupId = result.get(task.attributableURL) || {};
+      const originalTime = timingByGroupId[task.group.id] || 0;
+      timingByGroupId[task.group.id] = originalTime + task.selfTime * multiplier;


ha, it reads better inlined to me too

brendankenny · 2018-06-26T21:27:07Z

lighthouse-core/gather/computed/main-thread-tasks.js

+ * Each task will have its group/classification, start time, end time,
+ * duration, and self time computed. Each task will potentially have a parent, children, and an
+ * attributeableURL for the script that was executing/forced this execution.
+ */


brendankenny · 2018-06-26T21:36:12Z

lighthouse-core/gather/computed/main-thread-tasks.js

+      MainThreadTasks._computeRecursiveTaskGroup(task);
+    }
+
+    const firstTs = (tasks[0] || {startTime: 0}).startTime;


oh yeah, we should make sure to use the sorted events here 👍

should this depend on traceOfTab.mainThreadEvents? Then it doesn't need to check if tasks are in the main thread and they're already sorted

brendankenny · 2018-06-26T21:37:36Z

lighthouse-core/gather/computed/main-thread-tasks.js

+      MainThreadTasks._computeRecursiveTaskGroup(task);
+    }
+
+    const firstTs = (tasks[0] || {startTime: 0}).startTime;


does this need the {startTime: 0} fallback?

patrickhulce · 2018-06-27T21:43:15Z

@paulirish still requested changes?

brendankenny

LGTM!

brendankenny · 2018-06-27T22:24:48Z

lighthouse-core/gather/computed/main-thread-tasks.js

+        break;
+    }
+
+    /** @type {string[]} */


nit: not needed

brendankenny · 2018-06-27T22:27:16Z

lighthouse-core/gather/computed/main-thread-tasks.js

+ *
+ * Each task will have its group/classification, start time, end time,
+ * duration, and self time computed. Each task will potentially have a parent, children, and an
+ * attributableURL for the script that was executing/forced this execution.


should this be attributableURLs, plural? What does it mean when there's more than one?

brendankenny · 2018-06-27T22:28:28Z

Removes 233 KB (48 KB after gzip) from our bundle!

paulirish · 2018-06-27T21:57:40Z

lighthouse-core/gather/computed/main-thread-tasks.js

+
+    let taskURLs = [];
+    switch (task.event.name) {
+      case 'v8.compile':


can we use the names from task-groups? (or at least add a comment about where these come from)

yeah I'll add a comment :)

paulirish · 2018-06-27T22:00:39Z

lighthouse-core/test/gather/computed/main-thread-tasks-test.js

+    });
+  });
+
+  it('should compute attributableURLs correclty', async () => {


plz spell correctly correctly

paulirish · 2018-06-27T23:53:39Z

lighthouse-core/test/gather/computed/main-thread-tasks-test.js

+    const baseTs = 1241250325;
+    const url = s => ({args: {data: {url: s}}});
+    const stackFrames = f => ({args: {data: {stackTrace: f.map(url => ({url}))}}});
+


here i made this for you

/* An artistic rendering of the below trace: █████████████████████████████TaskA██████████████████████████████████████████████ ████████████████TaskB███████████████████ ████EvaluateScript██████ █D█ */

thanks 🙏

paulirish · 2018-06-28T00:06:18Z

lighthouse-core/test/gather/computed/main-thread-tasks-test.js

+  });
+
+  it('should compute parent/child correctly', async () => {
+    const traceEvents = [


here i made this for you:

/* An artistic rendering of the below trace: █████████████████████████████TaskA██████████████████████████████████████████████ ████████████████TaskB███████████████████ ███████TaskC██████████ */

i also made an x-axis but it scale correctly against the box-drawing characters in any monospace fonts except the one i personally use. :(

|----|-----|---|---|------------------|-----------------|--------------------------------------------|- 0 5 10 15 20 40 55 100

paulirish · 2018-06-28T17:35:53Z

core: remove dependency on DevtoolsTimelineModel

e2162f5

patrickhulce requested review from brendankenny and paulirish as code owners June 20, 2018 23:16

golden lhr

b15c188

patrickhulce mentioned this pull request Jun 21, 2018

core(trace-of-tab): remove DevTools stableSort dependency #5532

Merged

brendankenny reviewed Jun 22, 2018

View reviewed changes

patrickhulce added 4 commits June 25, 2018 09:42

feedback

279e49d

moar docs

155b3ad

golden lhr

657f7de

Merge branch 'master' into remove_dtm_dep

cec72cd

paulirish requested changes Jun 26, 2018

View reviewed changes

brendankenny reviewed Jun 26, 2018

View reviewed changes

stack of attributableURLs

99ab3e1

paulirish added the 3.0 label Jun 27, 2018

patrickhulce added 2 commits June 27, 2018 13:41

Merge branch 'master' into remove_dtm_dep

6dd725e

rebase

d2def08

brendankenny approved these changes Jun 27, 2018

View reviewed changes

paulirish approved these changes Jun 28, 2018

View reviewed changes

last feedback

d712444

patrickhulce merged commit 341f0a8 into master Jun 28, 2018

patrickhulce deleted the remove_dtm_dep branch June 28, 2018 17:53

patrickhulce mentioned this pull request Jul 10, 2018

bootup time is missing execution time #5325

Closed

paulirish removed 3.0 labels Dec 18, 2018

core: remove dependency on DevtoolsTimelineModel #5533

core: remove dependency on DevtoolsTimelineModel #5533

Conversation

patrickhulce commented Jun 20, 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

Choose a reason for hiding this comment

patrickhulce Jun 25, 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

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 Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

patrickhulce commented Jun 26, 2018

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

Choose a reason for hiding this comment

patrickhulce commented Jun 27, 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

brendankenny commented Jun 27, 2018

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 Jun 28, 2018

patrickhulce commented Jun 20, 2018 •

edited

Loading

patrickhulce Jun 25, 2018 •

edited

Loading

brendankenny Jun 25, 2018 •

edited

Loading