new_audit: add long-tasks diagnostic audit for surfacing top long tasks #10736

Beytoven · 2020-05-08T23:01:38Z

Addresses #10705

connorjclark · 2020-05-08T23:12:38Z

@brendankenny @patrickhulce I assume main-thread-tasks is for programmatic usages via HTTP Archive. true? Do we want to keep it? If so, @Beytoven we'll need to make a new audit instead main-thread-long-tasks ...

patrickhulce · 2020-05-08T23:49:54Z

Ooh, yeah good catch @connorjclark. It'd be a shame to lose this data. Not only for HA but it's a great diagnostic tool as well. I'm not sure if the visualizations in calibre and such use this or the trace itself.

Would you be able to create a long-tasks specific one @Beytoven ?

connorjclark · 2020-05-12T00:09:48Z

lighthouse-core/audits/long-tasks.js

+
+const UIStrings = {
+  /** Title of a diagnostic LH audit that provides details on the longest running tasks that occur when the page loads. */
+  title: 'Avoids long-running tasks',


Since we're just listing the tasks here and not scoring the audit, title should be more declarative than prescriptive: Long main thread tasks should be fine.

description could be Lists the longest tasks on the main thread, useful for identifying worst contributors to input delay ?

Do we need any sort of web.dev url for this? maybe just link to web.dev/tbt ?

I thought about linking to tbt, but unsure as I don't want this audit to be seen as specifically for tbt.

lighthouse-core/audits/long-tasks.js

connorjclark · 2020-05-12T00:13:53Z

lighthouse-core/audits/long-tasks.js

+
+    const jsURLs = LongTasks.getJavaScriptURLs(networkRecords);
+    const longtasks = [...tasks].sort((a, b) => b.duration - a.duration)
+      .filter(t => t.duration >= 50 && t.unbounded === false && !t.parent)


filter, then sort (less work)

connorjclark · 2020-05-12T00:14:10Z

lighthouse-core/audits/long-tasks.js

+      .filter(t => t.duration >= 50 && t.unbounded === false && !t.parent)
+      .slice(0, 20);
+
+    const results = longtasks.map(t => ({


connorjclark · 2020-05-12T00:16:41Z

lighthouse-core/audits/long-tasks.js

+   * @param {Set<string>} jsURLs
+   * @return {string}
+   */
+  static getAttributableURLForTask(task, jsURLs) {


this is BootupTime.getAttributableURLForTask right? use that instead?

lighthouse-core/audits/long-tasks.js

connorjclark · 2020-05-12T00:21:05Z

lighthouse-core/lib/i18n/locales/uk.json

@@ -1136,12 +1136,6 @@
  "lighthouse-core/audits/timing-budget.js | title": {
    "message": "Обмеження часу"
  },
-  "lighthouse-core/audits/user-timings.js | columnDuration": {


FYI we will need a string import when this lands @exterkamp

lighthouse-core/test/audits/long-tasks-test.js

Co-authored-by: Connor Clark <cjamcl@google.com>

connorjclark · 2020-05-12T02:06:16Z

can you add a displayValue that says "1 long task found" / "3 long tasks found" etc., like for the layout shift audit?

after that I think we're good here

patrickhulce

looks great! nice job @Beytoven!

just a few data inclusion and testing nits but LGTM 🎉

patrickhulce · 2020-05-26T17:47:07Z

lighthouse-cli/test/smokehouse/test-definitions/perf/expectations.js

@@ -145,62 +169,35 @@ module.exports = [
                node: {
                  type: 'node',
                  nodeLabel: 'img',
-                  path: '2,HTML,1,BODY,0,IMG',
+                  path: '0,HTML,1,BODY,0,DIV,0,IMG',


maybe we should use \d to make this less brittle?

I'd prefer removing path and instead adding something like src or another unique attribute here (can we match for that in snippet?)

On a quest to reduce paths in smoke tests :) https://github.com/GoogleChrome/lighthouse/pull/10827/files

lighthouse-cli/test/fixtures/perf/delayed-element.js

lighthouse-core/test/audits/long-tasks-test.js

patrickhulce · 2020-05-26T18:01:04Z

lighthouse-core/test/audits/long-tasks-test.js

+    }
+  });
+
+  it('should populate url when tasks have an attributable url', async () => {


nice touch :)

patrickhulce · 2020-05-26T18:02:08Z

lighthouse-core/audits/long-tasks.js

+      url: BootupTime.getAttributableURLForTask(task, jsURLs),
+      group: task.group.label,
+      start: task.startTime,
+      self: task.selfTime,


I'm not sure selfTime is worth including for the toplevel task since anything there is just overhead.

If we include it should we ensure it matches the multiplier too and throw a test on it?

patrickhulce · 2020-05-26T18:04:10Z

lighthouse-core/audits/long-tasks.js

+    const results = longtasks.map(task => ({
+      url: BootupTime.getAttributableURLForTask(task, jsURLs),
+      group: task.group.label,
+      start: task.startTime,


I think it's going to be weird that startTime is reported on the actual timeline while duration is going to be on the simulated timeline, should we just exclude it for now?

patrickhulce · 2020-05-26T18:04:46Z

lighthouse-core/audits/long-tasks.js

+
+    const results = longtasks.map(task => ({
+      url: BootupTime.getAttributableURLForTask(task, jsURLs),
+      group: task.group.label,


don't know if group is worth including when it's value is always Other, maybe we exclude it?

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

lighthouse-core/audits/long-tasks.js

connorjclark · 2020-05-27T19:03:36Z

lighthouse-core/lib/i18n/i18n.js

@@ -84,6 +84,10 @@ const UIStrings = {
  columnOverBudget: 'Over Budget',
  /** Label for a column in a data table; entries will be a representation of a DOM element. */
  columnElement: 'Element',
+  /** Label for a column in a data table; entries will be the number of milliseconds since the page started loading. */
+  columnStartTime: 'Start Time',


technically not needed here but I say keep it b/c it will be needed here in your followup

connorjclark · 2020-05-27T19:07:00Z

lighthouse-cli/test/smokehouse/test-definitions/perf/expectations.js

      },
    },
  },
-  // TODO: uncomment when Chrome m83 lands


I'm ok with removing this but can you make an issue + assign to yourself so we don't forget to add it back when m84 (comment is wrong) lands?

connorjclark · 2020-05-27T19:07:37Z

lighthouse-cli/test/smokehouse/test-definitions/perf/expectations.js

+    lhr: {
+      requestedUrl: 'http://localhost:10200/perf/trace-elements.html',
+      finalUrl: 'http://localhost:10200/perf/trace-elements.html',
+      audits: {
        'largest-contentful-paint-element': {
          score: null,
          displayValue: '1 element found',


driveby: can you remove displayValue?

connorjclark · 2020-05-27T19:08:10Z

lighthouse-core/test/audits/long-tasks-test.js

+      devtoolsLogs: {defaultPass: devtoolsLog},
+    };
+    const result = await LongTasks.audit(artifacts, {computedCache: new Map()});
+    // expect(result.score).toBe(0);


connorjclark · 2020-05-27T19:10:16Z

lighthouse-core/test/audits/long-tasks-test.js

+ */
+function generateTraceWithLongTasks(
+  count,
+  duration = 200,


styling nit: we tend to only have line breaks in params if it is too long.

connorjclark · 2020-05-27T19:12:13Z

lighthouse-core/test/audits/long-tasks-test.js

+    const task = {ts, duration};
+    task.children = [];
+    if (withChildTasks) {
+      task.children.push({


Can you add a comment detailing (or maybe an asci chart) the structure of the child tasks?

Good idea, will do.

connorjclark · 2020-05-27T19:12:41Z

lighthouse-core/test/audits/long-tasks-test.js

+  const baseTs = 1000;
+  const traceTasks = [];
+  for (let i = 1; i <= count; i++) {
+    const ts = baseTs * i;


looks like tasks start at 1s instead of 0, is that important?

For this case, yes. I want to avoid any overlapping trace events so that no unintentional parent/child task relationships get formed since we need top level tasks for this audit. So starting at 1s avoids any of the added tasks overlapping with the required tasks defined in CreateTestTrace().

connorjclark · 2020-05-27T19:13:47Z

lighthouse-core/test/audits/long-tasks-test.js

+  });
+
+  it('should populate url when tasks have an attributable url', async () => {
+    const trace = generateTraceWithLongTasks(1, 300, true);


maybe use object parameter? can't tell what these values mean w/o looking at function.

Co-authored-by: Connor Clark <cjamcl@google.com>

…s own PR

Update main thread tasks to surface long tasks

2b9e91b

Beytoven requested a review from a team as a code owner May 8, 2020 23:01

Beytoven requested review from paulirish and removed request for a team May 8, 2020 23:01

googlebot added the cla: yes label May 8, 2020

devtools-bot assigned paulirish May 11, 2020

devtools-bot added the waiting4reviewer label May 11, 2020

brendankenny mentioned this pull request May 11, 2020

[meta] Lighthouse 6.0 Burndown #9774

Closed

54 tasks

Move long tasks into it's own audit

09eae76

vercel bot deployed to Preview May 11, 2020 23:09 View deployment

Beytoven changed the title ~~core(main-thread-tasks): surface top long tasks as a diagnostic~~ core: new long-tasks diagnostic audit for surfacing top long tasks May 11, 2020

connorjclark reviewed May 12, 2020

View reviewed changes

lighthouse-core/audits/long-tasks.js Outdated Show resolved Hide resolved

connorjclark reviewed May 12, 2020

View reviewed changes

lighthouse-core/audits/long-tasks.js Outdated Show resolved Hide resolved

connorjclark reviewed May 12, 2020

View reviewed changes

lighthouse-core/test/audits/long-tasks-test.js Outdated Show resolved Hide resolved

connorjclark reviewed May 12, 2020

View reviewed changes

lighthouse-core/test/audits/long-tasks-test.js Outdated Show resolved Hide resolved

Apply suggestions from code review

3104257

Co-authored-by: Connor Clark <cjamcl@google.com>

vercel bot deployed to Preview May 12, 2020 01:08 View deployment

Address PR comments

d9a42b9

vercel bot deployed to Preview May 12, 2020 01:20 View deployment

Merge changes from master

058d758

vercel bot deployed to Preview May 12, 2020 01:32 View deployment

Add displayValue to long tasks audit

7adfd60

Beytoven added the waiting4reviewer label May 21, 2020

patrickhulce approved these changes May 26, 2020

View reviewed changes

Apply suggestions from Patrick

0aa679a

Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

vercel bot deployed to Preview May 26, 2020 23:21 View deployment

patrickhulce added waiting4committer and removed waiting4reviewer labels May 27, 2020

Addressing comments; Update smoketest checks

45f7507

vercel bot deployed to Preview May 27, 2020 18:42 View deployment

connorjclark reviewed May 27, 2020

View reviewed changes

lighthouse-core/audits/long-tasks.js Outdated Show resolved Hide resolved

connorjclark reviewed May 27, 2020

View reviewed changes

Update lighthouse-core/audits/long-tasks.js

86955fa

Co-authored-by: Connor Clark <cjamcl@google.com>

vercel bot deployed to Preview May 27, 2020 19:38 View deployment

Fix unit tests; address more comments

1076fe1

vercel bot deployed to Preview May 27, 2020 23:48 View deployment

Re-comment out the CLS element smoke test

16db613

vercel bot deployed to Preview May 28, 2020 07:51 View deployment

Lint fix

59fc2d1

vercel bot deployed to Preview May 28, 2020 17:47 View deployment

Adding back displayValue for LCP Element; moving those changes to it'…

8d5b32f

…s own PR

vercel bot deployed to Preview May 28, 2020 18:51 View deployment

Beytoven merged commit 1199d72 into master May 28, 2020

Beytoven deleted the long-tasks branch May 28, 2020 20:45

connorjclark added the new_audit label Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit: add long-tasks diagnostic audit for surfacing top long tasks #10736

new_audit: add long-tasks diagnostic audit for surfacing top long tasks #10736

Beytoven commented May 8, 2020

connorjclark commented May 8, 2020

patrickhulce commented May 8, 2020

connorjclark May 12, 2020 •

edited

Loading

Beytoven May 12, 2020

connorjclark May 12, 2020

connorjclark May 12, 2020

connorjclark May 12, 2020

connorjclark May 12, 2020

connorjclark commented May 12, 2020 •

edited

Loading

patrickhulce left a comment

patrickhulce May 26, 2020

connorjclark May 26, 2020

patrickhulce May 26, 2020

patrickhulce May 26, 2020

patrickhulce May 26, 2020

patrickhulce May 26, 2020

connorjclark May 27, 2020

connorjclark May 27, 2020

connorjclark May 27, 2020

connorjclark May 27, 2020

connorjclark May 27, 2020

connorjclark May 27, 2020

Beytoven May 27, 2020

connorjclark May 27, 2020

Beytoven May 27, 2020

connorjclark May 27, 2020

new_audit: add long-tasks diagnostic audit for surfacing top long tasks #10736

new_audit: add long-tasks diagnostic audit for surfacing top long tasks #10736

Conversation

Beytoven commented May 8, 2020

connorjclark commented May 8, 2020

patrickhulce commented May 8, 2020

connorjclark May 12, 2020 • 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

connorjclark commented May 12, 2020 • edited Loading

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

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

connorjclark May 12, 2020 •

edited

Loading

connorjclark commented May 12, 2020 •

edited

Loading