Add TRACE_EVENT_IS_NEW_TRACE as a way to snapshot objects at start of recording

Add TRACE_EVENT_IS_NEW_TRACE as a way to snapshot objects at start of recording

R=dsinclair,enne
NOTRY=True

Android test failures are flake.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203810

[dsinclair, 2013-05-31 21:16:02]

Lgtm
On May 31, 2013 5:06 PM, <nduca@chromium.org> wrote:

> Reviewers: dsinclair,
>
> Description:
> Add TRACE_EVENT_IS_NEW_TRACE as a way to snapshot objects at start of
> recording
>
> R=dsinclair
>
> Please review this at
https://codereview.chromium.**org/15774010/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M base/debug/trace_event.h
>   M base/debug/trace_event_impl.h
>   M base/debug/trace_event_impl.cc
>   M base/debug/trace_event_**unittest.cc
>
>
> Index: base/debug/trace_event.h
> diff --git a/base/debug/trace_event.h b/base/debug/trace_event.h
> index abd3901b9bbd44b13ffad4278107fc**6f4757023e..**
> aa4cc759b79eeabca051ff795c0745**5cf56eb3c2 100644
> --- a/base/debug/trace_event.h
> +++ b/base/debug/trace_event.h
> @@ -678,6 +678,22 @@
>        } \
>      } while (0)
>
> +// Macro to efficiently determine, through polling, if a new trace has
> begun.
> +#define TRACE_EVENT_IS_NEW_TRACE(ret) \
> +    do { \
> +      static int INTERNAL_TRACE_EVENT_UID(**lastRecordingNumber) = 0; \
> +      int num_traces_recorded = TRACE_EVENT_API_GET_NUM_**TRACES_RECORDED();
> \
> +      if (num_traces_recorded != -1 && \
> +          num_traces_recorded != \
> +          INTERNAL_TRACE_EVENT_UID(**lastRecordingNumber)) { \
> +        INTERNAL_TRACE_EVENT_UID(**lastRecordingNumber) = \
> +            num_traces_recorded; \
> +        *ret = true; \
> +      } else { \
> +        *ret = false; \
> +      } \
> +    } while (0)
> +
>  //////////////////////////////**//////////////////////////////**
> ////////////////////
>  // Implementation specific tracing API definitions.
>
> @@ -694,6 +710,12 @@
>  #define TRACE_EVENT_API_GET_CATEGORY_**GROUP_ENABLED \
>      base::debug::TraceLog::**GetCategoryGroupEnabled
>
> +// Get the number of times traces have been recorded. This is used to
> implement
> +// the TRACE_EVENT_IS_NEW_TRACE facility.
> +// unsigned int TRACE_EVENT_API_GET_NUM_**TRACES_RECORDED()
> +#define TRACE_EVENT_API_GET_NUM_**TRACES_RECORDED \
> +    base::debug::TraceLog::**GetInstance()->**GetNumTracesRecorded
> +
>  // Add a trace event to the platform tracing system.
>  // void TRACE_EVENT_API_ADD_TRACE_**EVENT(
>  //                    char phase,
> Index: base/debug/trace_event_impl.cc
> diff --git a/base/debug/trace_event_impl.**cc
> b/base/debug/trace_event_impl.**cc
> index e8b3083339cb14a72a2e338f2857bb**fc8063f9f0..**
> bbe99ac8cc4fb74417009878b5928a**0fd8a37fae 100644
> --- a/base/debug/trace_event_impl.**cc
> +++ b/base/debug/trace_event_impl.**cc
> @@ -773,6 +773,7 @@ TraceLog::Options
TraceLog::**TraceOptionsFromString(const
> std::string& options) {
>
>  TraceLog::TraceLog()
>      : enable_count_(0),
> +      num_traces_recorded_(0),
>        logged_events_(NULL),
>        dispatching_to_observer_list_(**false),
>        watch_category_(NULL),
> @@ -927,6 +928,8 @@ void TraceLog::SetEnabled(const CategoryFilter&
> category_filter,
>      return;
>    }
>
> +  num_traces_recorded_++;
> +
>    dispatching_to_observer_list_ = true;
>    FOR_EACH_OBSERVER(**EnabledStateChangedObserver,
> enabled_state_observer_list_,
>                      OnTraceLogWillEnable());
> @@ -1001,6 +1004,13 @@ void TraceLog::SetDisabled() {
>    AddThreadNameMetadataEvents();
>  }
>
> +int TraceLog::**GetNumTracesRecorded() {
> +  AutoLock lock(lock_);
> +  if (enable_count_ == 0)
> +    return -1;
> +  return num_traces_recorded_;
> +}
> +
>  void TraceLog::**AddEnabledStateObserver(**EnabledStateChangedObserver*
> listener) {
>    enabled_state_observer_list_.**AddObserver(listener);
>  }
> @@ -1511,4 +1521,3 @@ ScopedTrace::~ScopedTrace() {
>  }
>
>  }  // namespace trace_event_internal
> -
> Index: base/debug/trace_event_impl.h
> diff --git a/base/debug/trace_event_impl.**h
> b/base/debug/trace_event_impl.**h
> index e6bca4b324f7f2d8aa027d996047fb**6fcfa9b1a3..**
> 3acff51d4cedef3404357064584039**0eb07788d1 100644
> --- a/base/debug/trace_event_impl.**h
> +++ b/base/debug/trace_event_impl.**h
> @@ -317,6 +317,13 @@ class BASE_EXPORT TraceLog {
>    void SetDisabled();
>    bool IsEnabled() { return !!enable_count_; }
>
> +  // The number of times we have begun recording traces. If tracing is
> off,
> +  // returns -1. If tracing is on, then it returns the number of times we
> have
> +  // recorded a trace. By watching for this number to increment, you can
> +  // passively discover when a new trace has begun. This is then used to
> +  // implement the TRACE_EVENT_IS_NEW_TRACE() primitive.
> +  int GetNumTracesRecorded();
> +
>  #if defined(OS_ANDROID)
>    void StartATrace();
>    void StopATrace();
> @@ -524,6 +531,7 @@ class BASE_EXPORT TraceLog {
>    // This lock protects TraceLog member accesses from arbitrary threads.
>    Lock lock_;
>    int enable_count_;
> +  int num_traces_recorded_;
>    NotificationCallback notification_callback_;
>    scoped_ptr<TraceBuffer> logged_events_;
>    EventCallback event_callback_;
> Index: base/debug/trace_event_**unittest.cc
> diff --git a/base/debug/trace_event_**unittest.cc
> b/base/debug/trace_event_**unittest.cc
> index 26e79c076ca73c176d4d9479582a89**4d9f08ad55..**
> 02bb899d2a227d4258a3b565c4aec0**0908b14b4e 100644
> --- a/base/debug/trace_event_**unittest.cc
> +++ b/base/debug/trace_event_**unittest.cc
> @@ -905,6 +905,37 @@ TEST_F(TraceEventTestFixture,
> EnabledObserverFiresOnDisable) {
>    TraceLog::GetInstance()->**RemoveEnabledStateObserver(&**observer);
>  }
>
> +bool IsNewTrace() {
> +  bool is_new_trace;
> +  TRACE_EVENT_IS_NEW_TRACE(&is_**new_trace);
> +  return is_new_trace;
> +}
> +
> +TEST_F(TraceEventTestFixture, NewTraceRecording) {
> +  ManualTestSetUp();
> +  ASSERT_FALSE(IsNewTrace());
> +  TraceLog::GetInstance()->**SetEnabled(CategoryFilter("*")**,
> +                                      TraceLog::RECORD_UNTIL_FULL);
> +  // First call to IsNewTrace() should succeed. But, the second shouldn't.
> +  ASSERT_TRUE(IsNewTrace());
> +  ASSERT_FALSE(IsNewTrace());
> +  EndTraceAndFlush();
> +
> +  // IsNewTrace() should definitely be false now.
> +  ASSERT_FALSE(IsNewTrace());
> +
> +  // Start another trace. IsNewTrace() should become true again, briefly,
> as
> +  // before.
> +  TraceLog::GetInstance()->**SetEnabled(CategoryFilter("*")**,
> +                                      TraceLog::RECORD_UNTIL_FULL);
> +  ASSERT_TRUE(IsNewTrace());
> +  ASSERT_FALSE(IsNewTrace());
> +
> +  // Cleanup.
> +  EndTraceAndFlush();
> +}
> +
> +
>  // Test that categories work.
>  TEST_F(TraceEventTestFixture, Categories) {
>    ManualTestSetUp();
>
>
>

[commit-bot: I haz the power, 2013-05-31 21:17:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/15774010/1

[nduca, 2013-05-31 21:21:49]

Enne, mind if i fold the cc edits into the base bit?

[nduca, 2013-05-31 23:20:52]

ptal

[enne (OOO), 2013-05-31 23:27:20]

https://codereview.chromium.org/15774010/diff/3004/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/15774010/diff/3004/cc/layers/layer_impl.cc#ne...
cc/layers/layer_impl.cc:949: void LayerImpl::DidBeginTracing() {
No more roll-your-own-layer-recursion.  Please use CallFunctionForSubtree.  See
also: https://codereview.chromium.org/15959012/

https://codereview.chromium.org/15774010/diff/3004/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/15774010/diff/3004/cc/resources/picture.cc#ne...
cc/resources/picture.cc:392:
TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
What about making this Picture::EmitTraceSnapshot or something and calling it
from CloneForDrawing too so that it's not repeated?

https://codereview.chromium.org/15774010/diff/3004/cc/resources/picture_pile_...
File cc/resources/picture_pile_impl.cc (right):

https://codereview.chromium.org/15774010/diff/3004/cc/resources/picture_pile_...
cc/resources/picture_pile_impl.cc:382: for (TilingData::Iterator
tile_iter(&tiling_, layer_rect);
It might be shorter to just iterate through the picture map and then each list
in the map.  No reason to use TilingData unless there's geometry involved.

[nduca, 2013-06-01 02:19:43]

all set, thanks for the feedback!

[enne (OOO), 2013-06-01 03:10:45]

lgtm

https://codereview.chromium.org/15774010/diff/11001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/15774010/diff/11001/cc/resources/picture.cc#n...
cc/resources/picture.cc:389: void Picture::DidBeginTracing() {
I'm not sure this function really needs to exist.

[commit-bot: I haz the power, 2013-06-02 18:44:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/15774010/36001

[commit-bot: I haz the power, 2013-06-02 18:44:46]

Failed to apply patch for cc/resources/picture.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/resources/picture.cc
  Hunk #1 FAILED at 225.
  Hunk #2 succeeded at 276 (offset 3 lines).
  Hunk #3 succeeded at 390 (offset 3 lines).
  1 out of 3 hunks FAILED -- saving rejects to file cc/resources/picture.cc.rej

Patch:       cc/resources/picture.cc
Index: cc/resources/picture.cc
diff --git a/cc/resources/picture.cc b/cc/resources/picture.cc
index
b0f3fa30e59f0587759f402efd64fd6fe71392da..71c72ead34e89ce2c7f003144e5658a16acd9305
100644
--- a/cc/resources/picture.cc
+++ b/cc/resources/picture.cc
@@ -225,8 +225,7 @@ void Picture::CloneForDrawing(int num_threads) {
                     pixel_refs_));
     clones_.push_back(clone);
 
-    TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-      "cc::Picture", clone.get(), TracedPicture::AsTraceablePicture(clone));
+    clone->EmitTraceSnapshot();
   }
 }
 
@@ -273,8 +272,7 @@ void Picture::Record(ContentLayerClient* painter,
 
   opaque_rect_ = gfx::ToEnclosedRect(opaque_layer_rect);
 
-    TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
-      "cc::Picture", this, TracedPicture::AsTraceablePicture(this));
+  EmitTraceSnapshot();
 }
 
 void Picture::GatherPixelRefs(
@@ -388,6 +386,11 @@ scoped_ptr<Value> Picture::AsValue() const {
   return res.PassAs<base::Value>();
 }
 
+void Picture::EmitTraceSnapshot() {
+  TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
+      "cc::Picture", this, TracedPicture::AsTraceablePicture(this));
+}
+
 base::LazyInstance<Picture::PixelRefs>
     Picture::PixelRefIterator::empty_pixel_refs_;

[commit-bot: I haz the power, 2013-06-03 17:05:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/15774010/39001

[commit-bot: I haz the power, 2013-06-04 01:31:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/15774010/39001

[commit-bot: I haz the power, 2013-06-04 01:31:53]

Change committed as 203810