New issue
Advanced search Search tips

Issue 659689 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 626326



Sign in to add a comment

Tracker bug for tracing refactoring and unbundling

Project Member Reported by primiano@chromium.org, Oct 26 2016

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Nov 10 2016

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

commit 094ac7f87d78a80e3934e148855a908977559166
Author: primiano <primiano@chromium.org>
Date: Thu Nov 10 18:43:08 2016

tracing: split out the CategoryRegistry from the TraceLog

Changes introduced by this CL:
 - Split out the category logic out of the TraceLog into CategoryRegistry.
   Makes it easier to unbundle, reuse and reason about.

 - Get rid of the parallel arrays for tracking names, filters and state
   of each category and switch them to one array of struct. This not
   only makes the code easier to read, but also opens the way for
   having categories defined at compile time that don't need any
   lazy initialization of the "enabled" ptr.

 - Naming cleanup: the term "category group" has always been confusing.
   All it does is reflecting a subtle implementation detail (the fact
   that a category name can be "cat1,cat2") which is quite irrelevant
   from a functional viewpoint.

BUG=659689

Review-Url: https://codereview.chromium.org/2452063003
Cr-Commit-Position: refs/heads/master@{#431300}

[modify] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/BUILD.gn
[add] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/category_registry.cc
[add] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/category_registry.h
[add] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/trace_category.h
[add] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/trace_category_unittest.cc
[modify] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/trace_event.h
[modify] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/trace_log.cc
[modify] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/base/trace_event/trace_log.h
[modify] https://crrev.com/094ac7f87d78a80e3934e148855a908977559166/tools/gn/bootstrap/bootstrap.py

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 21 2016

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

commit 5a5b09bbda9a7399e602b4cc01e0c56bb187e651
Author: primiano <primiano@chromium.org>
Date: Mon Nov 21 19:16:23 2016

tracing: fix races in CategoryRegistry

The previous CL crrev.com/2452063003 did introduce a subtle race and
a more trivial test flakiness.

1) race in trace_log.cc: this is a classical time-of-check vs
time-of-use from school books. The newly introduced
GetOrCreateCategoryByName() API  was badly designed. If two threads
hit that concurrently only one of the two would see a true retval and initialize
the category (good) but the other one will carry on and use the uninitialized
category before the first one has completed the initialization from the
TraceLog (bad).
This was a consequence of trying to separate the locks between TraceLog
and CategoryRegistry. The new design is simpler and more aligned with
the old behavior. TraceLog is the only one handling a lock and
CategoryRegistry expects the caller to  serialize calls, exposing an
explicit GetOrCreateCategoryLocked method.

2) flakiness in trace_category_unittest.cc: categories cannot be
reset once they are created, even in tests. This is because
the static pointers injected by the TRACE_EVENT macros cannot
be reset once they have been initialized so once hit a category
pointer must be stable for the entire lifetime of the process (this
behavior predates crrev.com/2452063003 and isn't a recent change).
Concretely this is causing an interference between the two new
test fixtures recently introduced in crrev.com/2452063003 if they are
ran in reverse order. Fixed by changing the prefix of the category
names so they don't collide.

BUG=659689

Review-Url: https://codereview.chromium.org/2495173002
Cr-Commit-Position: refs/heads/master@{#433601}

[modify] https://crrev.com/5a5b09bbda9a7399e602b4cc01e0c56bb187e651/base/trace_event/category_registry.cc
[modify] https://crrev.com/5a5b09bbda9a7399e602b4cc01e0c56bb187e651/base/trace_event/category_registry.h
[modify] https://crrev.com/5a5b09bbda9a7399e602b4cc01e0c56bb187e651/base/trace_event/trace_category_unittest.cc
[modify] https://crrev.com/5a5b09bbda9a7399e602b4cc01e0c56bb187e651/base/trace_event/trace_log.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 10 2016

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

commit a0835eec90a7fa97ea838e93540135115aa833a3
Author: primiano <primiano@chromium.org>
Date: Sat Dec 10 02:04:14 2016

tracing: split trace event filter classes out of TraceLog

Another refactoring aimed at slimming down the TraceLog monster-class
and isolating dependencies of the tracing codebase.
This CL is a pure refactor with no intended behavioral changes.
It moves all the trace filters classes outside of the TraceLog.
This is required to make the next CLs which will change the lifetime
management of filters easier to review.

BUG=659689

Review-Url: https://codereview.chromium.org/2549103003
Cr-Commit-Position: refs/heads/master@{#437721}

[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/BUILD.gn
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/category_registry.h
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/event_name_filter.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/event_name_filter.h
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/event_name_filter_unittest.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/heap_profiler_event_filter.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/heap_profiler_event_filter.h
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/memory_dump_manager.cc
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_config.cc
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_config.h
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_config_unittest.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_event_filter.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_event_filter.h
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_event_filter_test_utils.cc
[add] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_event_filter_test_utils.h
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_event_unittest.cc
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_log.cc
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_log.h
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/base/trace_event/trace_log_constants.cc
[modify] https://crrev.com/a0835eec90a7fa97ea838e93540135115aa833a3/tools/gn/bootstrap/bootstrap.py

Components: Speed>Tracing
Components: -Internals>Tracing

Sign in to add a comment