New issue
Advanced search Search tips

Issue 899897 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 906734



Sign in to add a comment

[Tracker bug] Improve Slow Reports

Project Member Reported by gab@chromium.org, Oct 29

Issue description

This is a tracker bug for any CL required to improve Chromium tracing/debugging tools for Slow Reports.
 
Cc: erikc...@chromium.org ssid@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 2

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

commit 2f1e97157af097d62ac6084c7edc8f35a0cd2461
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Nov 02 19:40:23 2018

[TaskScheduler] Alive/dead instant events instead of begin/end

This is preferable because it avoids adding trace events while the
thread is idle (for which there is no precedent).

The delta between the instant events can be used to perform the same
analysis which could be performed previously (and these will be in
Slow Reports per being in an enabled category).

R=fdoray@chromium.org

Bug: 899897
Change-Id: I40d31df9f4db3ea8959f760323b7cd8f56299ce3
Reviewed-on: https://chromium-review.googlesource.com/c/1312928
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605017}
[modify] https://crrev.com/2f1e97157af097d62ac6084c7edc8f35a0cd2461/base/task/task_scheduler/scheduler_worker.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 7

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

commit 5db1ab8dc0279dc7d77b731e4815a57114dd8642
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Nov 07 05:14:06 2018

Add a trace event for the duration of ScopedBlockingCalls

This helps identify sections of a trace event that were blocked on external
dependencies and shed some light on the "descheduled" portion of an event.

This CL is non-trivial for 3 reasons:

 1) This new TRACE_EVENT kicks off tracing earlier than before in some
    configurations. This means that main() of basic utilities all of a sudden
    needed AtExitManagers (first patch set of this CL). But that then spread out
    to many main()'s and was painful. Since the Singleton is only required in
    tracing to know whether it's enabled : this CL specializes
    Singleton/TraceEventETWExport to avoid instantiating the instance only to
    notice it's not enabled (guaranteed default state).

 2) We do not want tracing events while a thread is idle (i.e. when it's
    sleeping on its own WaitableEvent/ConditionVariable while waiting for more
    tasks). Ideally we simply wouldn't record the trace event when it's not
    nested within another event but tracing doesn't have a notion of "current
    event depth". As such this CL opts for a declarative model where owners of
    the few WaitableEvents/ConditionVariables used to sleep-while-idle can flag
    those as irrelevant for tracing.

 3) Possibility of reentrancy if the trace event macros perform a blocking
    call themselves. Added DCHECKs to confirm this doesn't occur (was
    initial suspected cause of CrOS+amd64 only failures but it kept
    failing despite that and this feature had to be disabled there..)

Bonus:
  * Singleton::GetIfExists() avoids instantiating TraceEventETWExport
    when switches::kTraceExportEventsToETW isn't present.
  * The declarative model brought forth by (2) also enables not instantiating
    debug::ScopedEventWaitActivity objects while a thread is merely sleeping.
    This will avoid polluting crash metadata with wait-acitvity for sleeping
    threads.
  * In a follow-up CL we will be able to remove
    base::internal::ScopedClearBlockingObserverForTesting which was previously
    necessary specifically to avoid a situation in TaskScheduler unit tests
    where WaitableEvents used for the test logic would instantiate
    ScopedBlockingCalls and interfere with the very logic under test.
    (not done in this one as it's large enough on its own)

Bug: 899897,  902514 
Change-Id: I93b63e83ac6bd9e5e9d4e29a9b86c62c73738835
Reviewed-on: https://chromium-review.googlesource.com/c/1305891
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605966}
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/memory/singleton.h
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/message_loop/message_pump_default.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/condition_variable.h
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/condition_variable_posix.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/condition_variable_win.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/waitable_event.h
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/waitable_event_posix.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/synchronization/waitable_event_win.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/task/task_scheduler/scheduler_worker.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/threading/scoped_blocking_call.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/threading/scoped_blocking_call.h
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/trace_event/trace_event_etw_export_win.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/base/trace_event/trace_event_etw_export_win.h
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/cc/raster/single_thread_task_graph_runner.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/content/public/test/nested_message_pump_android.cc
[modify] https://crrev.com/5db1ab8dc0279dc7d77b731e4815a57114dd8642/content/renderer/categorized_worker_pool.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 13

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

commit ce06e2dffd1664b7f053fcd9fa9b3d2de7286a25
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Nov 13 01:08:09 2018

[Slow Reports] Whitelist v8.gc num_tasks/num_items args

R=etienneb@chromium.org, oysteine@chromium.org

Bug: 899897
Change-Id: I3fc881493ee2934ae15b43f4cb3089d98e460fcb
Reviewed-on: https://chromium-review.googlesource.com/c/1321189
Reviewed-by: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607398}
[modify] https://crrev.com/ce06e2dffd1664b7f053fcd9fa9b3d2de7286a25/chrome/common/trace_event_args_whitelist.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Nov 15

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

commit ed63e54f25c50ececfc9123ca9fd4574ec17c02f
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Nov 15 21:24:24 2018

[Slow Reports] Fix ScopedBlockingCall for waitable_event_mac.mm

This is a follow-up to https://chromium-review.googlesource.com/c/1305891

Missed that Mac had its own impl.
Example problematic trace because of this http://crash/18c6999290e01f89

Made all impls match waitable_event_win.cc
(also ignoring the ScopedEventWaitActivity when idle)

R=fdoray@chromium.org

Bug: 899897
Change-Id: Ib5aa1cec487f80a6f1d603658279666ee0d1de60
Reviewed-on: https://chromium-review.googlesource.com/c/1336073
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608511}
[modify] https://crrev.com/ed63e54f25c50ececfc9123ca9fd4574ec17c02f/base/synchronization/waitable_event_mac.cc
[modify] https://crrev.com/ed63e54f25c50ececfc9123ca9fd4574ec17c02f/base/synchronization/waitable_event_posix.cc
[modify] https://crrev.com/ed63e54f25c50ececfc9123ca9fd4574ec17c02f/base/synchronization/waitable_event_win.cc

Blockedon: 906734
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 20

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

commit 490bcd55c65198516ddf11b0c165bfe381588673
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Nov 20 16:40:19 2018

[Slow reports] Do not strip "blocking_type" tracing arg.

TBR=oysteine@chromium.org

Bug: 899897
Change-Id: Iaa1a9fbb2ae0d5122bfacb82a21f47e194221197
Reviewed-on: https://chromium-review.googlesource.com/c/1343177
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609726}
[modify] https://crrev.com/490bcd55c65198516ddf11b0c165bfe381588673/chrome/common/trace_event_args_whitelist.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jan 16 (6 days ago)

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

commit 40c2f1577996de8510142d8868a65621baaba1e2
Author: Andrey Malets <malets@yandex-team.ru>
Date: Wed Jan 16 14:29:40 2019

Fix thread cache after clone() in CanCreateProcessInNewUserNS

Since CanCreateProcessInNewUserNS uses base::ForkWithFlags() to create new
process, which in turn uses raw clone(), no function registered through
pthread_atfork() gets called in child.

This leads to invalid state when g_thread_id in platform_thread_posix.cc is
stale and PlatformThread::CurrentId() returns wrong thread id on Linux.

Despite the fact that the child process in CanCreateProcessInNewUserNS has very
short lifetime, it may use PlatformThread APIs when tracing was enabled at the
time of the call (this is the case when valid --trace-config-file= argument was
passed to Chrome at startup).

Also add a DCHECK to ensure we do not treat child crash and normal exit with
code 1 the same (this would have been very helpful to debug the error
originally).

Bug: 899897,  902514 
Change-Id: I8e2c82c8030537f5a62b725970d4ea5ec5948659
Reviewed-on: https://chromium-review.googlesource.com/c/1407830
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#623228}
[modify] https://crrev.com/40c2f1577996de8510142d8868a65621baaba1e2/base/process/launch_posix.cc
[modify] https://crrev.com/40c2f1577996de8510142d8868a65621baaba1e2/base/threading/platform_thread_internal_posix.h
[modify] https://crrev.com/40c2f1577996de8510142d8868a65621baaba1e2/base/threading/platform_thread_posix.cc
[modify] https://crrev.com/40c2f1577996de8510142d8868a65621baaba1e2/base/threading/scoped_blocking_call.cc
[modify] https://crrev.com/40c2f1577996de8510142d8868a65621baaba1e2/sandbox/linux/services/credentials.cc

Sign in to add a comment