New issue
Advanced search Search tips

Issue 874982 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 17
Components:
EstimatedDays: ----
NextAction: 2018-08-17
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 871204



Sign in to add a comment

Lighthouse fails to identify top-level tasks in Canary

Project Member Reported by paulir...@chromium.org, Aug 16

Issue description

Happened as a result of https://chromium-review.googlesource.com/1163611

This was landed and then merged to 69 here: https://bugs.chromium.org/p/chromium/issues/detail?id=871204#c10



We'll fix and roll Lighthouse to resolve
 
Labels: M-69
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 16

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0203ebc95b2bd29594826f483c8449298e1aca42

commit 0203ebc95b2bd29594826f483c8449298e1aca42
Author: Paul Irish <paulirish@chromium.org>
Date: Thu Aug 16 22:33:26 2018

DevTools: Roll Lighthouse for DoWork->RunTask trace event namechange

Bug:  874982 
Change-Id: I463cd1a8ea61407501732425a8dcb6363010bb87
Reviewed-on: https://chromium-review.googlesource.com/1178401
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Patrick Hulce <phulce@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583861}
[modify] https://crrev.com/0203ebc95b2bd29594826f483c8449298e1aca42/third_party/blink/renderer/devtools/front_end/audits2_worker/lighthouse/lighthouse-background.js

Labels: Merge-Request-69
pls apply appropriate OSs label.
Labels: OS-Chrome OS-Linux OS-Mac OS-Windows
NextAction: 2018-08-17
Pls update bug with canary result tomorrow and provide merge justification. Thank you.
The NextAction date has arrived: 2018-08-17
Canary result: 
Canary looks great. Reproduced the error in 70.0.3523.0 and then verified the fix in today's 70.0.3525.0

Justification:
Without this fix, the Audits panel in DevTools miscalculates all performance metrics and audits. It's due to the trace event name change in  issue 871204 . The Audit's panel continues to report numbers, but they're entirely incorrect. (Basically indicating the page is 90% faster than it is in reality).

 
Labels: -Merge-Request-69 Merge-Approved-69
Approving merge to M69 branch 3497 based on comment #9 and per offline chat with  paulirish@,  this is super contained code and won't have side effects. Pls merge. Thank you.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 17

Labels: -merge-approved-69 merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da701be4861e41da8799916ec74b44a9bfc8af0b

commit da701be4861e41da8799916ec74b44a9bfc8af0b
Author: Paul Irish <paulirish@chromium.org>
Date: Fri Aug 17 20:35:07 2018

DevTools: Roll Lighthouse for DoWork->RunTask trace event namechange

Bug:  874982 
Change-Id: I463cd1a8ea61407501732425a8dcb6363010bb87
Reviewed-on: https://chromium-review.googlesource.com/1178401
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Patrick Hulce <phulce@chromium.org>
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#583861}(cherry picked from commit 0203ebc95b2bd29594826f483c8449298e1aca42)
Reviewed-on: https://chromium-review.googlesource.com/1179738
Reviewed-by: Paul Irish <paulirish@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#695}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/da701be4861e41da8799916ec74b44a9bfc8af0b/third_party/blink/renderer/devtools/front_end/audits2_worker/lighthouse/lighthouse-background.js

Status: Fixed (was: Started)
Using a different trace event means we can also stop including the toplevel category, reducing the size of the trace: https://github.com/GoogleChrome/lighthouse/issues/3596

Project Member

Comment 14 by bugdroid1@chromium.org, Sep 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a87184b24c822c7c8c625408013409e9b4b9d3eb

commit a87184b24c822c7c8c625408013409e9b4b9d3eb
Author: Paul Irish <paulirish@chromium.org>
Date: Wed Sep 05 03:15:56 2018

Add ThreadControllerImpl::RunTask trace event in lighthouse category

Many tracing consumers collect the toplevel category only for this
event, however the toplevel category is very chatty (~25-50% of a
trace's bytes). This will allow downstream tools to collect much
smaller traces.

Bug:  874982 
Change-Id: Ib41ded90f120ba9a703a10f83320d0d10233ed13
Reviewed-on: https://chromium-review.googlesource.com/791314
Commit-Queue: Paul Irish <paulirish@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588753}
[modify] https://crrev.com/a87184b24c822c7c8c625408013409e9b4b9d3eb/base/task/sequence_manager/thread_controller_impl.cc

Project Member

Comment 15 by bugdroid1@chromium.org, Dec 17

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e587cacb10b4b7b703659ed3db46e015767fe7c

commit 5e587cacb10b4b7b703659ed3db46e015767fe7c
Author: Paul Irish <paulirish@chromium.org>
Date: Mon Dec 17 20:10:51 2018

sequence_manager: Add task traceevent for Lighthouse in message pump

Bug: 915301, 874982 
Change-Id: I1d67781b0a2da5a6682d4d2514feaedbfd641720
Reviewed-on: https://chromium-review.googlesource.com/c/1378815
Commit-Queue: Paul Irish <paulirish@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617204}
[modify] https://crrev.com/5e587cacb10b4b7b703659ed3db46e015767fe7c/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Dec 18

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba192d3e2ed2368379f858a1c8514ad82680f66b

commit ba192d3e2ed2368379f858a1c8514ad82680f66b
Author: Paul Irish <paulirish@chromium.org>
Date: Tue Dec 18 20:46:13 2018

sequence_manager: Add task traceevent for Lighthouse in message pump

TBR=paulirish@chromium.org

(cherry picked from commit 5e587cacb10b4b7b703659ed3db46e015767fe7c)

Bug: 915301, 874982 
Change-Id: I1d67781b0a2da5a6682d4d2514feaedbfd641720
Reviewed-on: https://chromium-review.googlesource.com/c/1378815
Commit-Queue: Paul Irish <paulirish@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#617204}
Reviewed-on: https://chromium-review.googlesource.com/c/1383146
Reviewed-by: Paul Irish <paulirish@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#446}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/ba192d3e2ed2368379f858a1c8514ad82680f66b/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc

Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/ba192d3e2ed2368379f858a1c8514ad82680f66b

Commit: ba192d3e2ed2368379f858a1c8514ad82680f66b
Author: paulirish@chromium.org
Commiter: paulirish@chromium.org
Date: 2018-12-18 20:46:13 +0000 UTC

sequence_manager: Add task traceevent for Lighthouse in message pump

TBR=paulirish@chromium.org

(cherry picked from commit 5e587cacb10b4b7b703659ed3db46e015767fe7c)

Bug: 915301, 874982 
Change-Id: I1d67781b0a2da5a6682d4d2514feaedbfd641720
Reviewed-on: https://chromium-review.googlesource.com/c/1378815
Commit-Queue: Paul Irish <paulirish@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#617204}
Reviewed-on: https://chromium-review.googlesource.com/c/1383146
Reviewed-by: Paul Irish <paulirish@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#446}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Labels: CommitLog-Audit-Violation Merge-Without-Approval M-72
Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision ba192d3e2ed2368379f858a1c8514ad82680f66b was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 
Labels: -CommitLog-Audit-Violation -Merge-Without-Approval

Sign in to add a comment