base: Make TraceEvent a movable class.

This is a small cleanup of the TraceEvent class performed
which is part of a larget bug allowing cleaning up and
reducing the generated machine code for TRACE_EVENTXXX()
macro calls (see related bug).

A first CL to perform this refactor was submitted as [1], but
later reverted because it made some tests fail mysteriously
(see http://crbug.com/899813). So the original CL was split into
several independent ones.

A first CL was submitted as [2], which actually fixed some
potential dangling pointer issues that were created from the
Initialize() and MoveFrom() methods.

This second CL removes this methods by making TraceEvent a
proper C++11 movable type, which should prevent (or at least
make it more difficult) introducing invalid states for its
instances. The goal is to see if this introduces new unexpected
test failures (which would indicate that there are still invalid
instance states used in the code base).

The third CL is [3] and re-introduces the TraceArguments helper
class on top of this one.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1318919
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1340308
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1318919

BUG=898794
R=primiano@chromium.org,oysteine@chromium.org,alexilin@chromium.org,chiniforooshan@chromium.org

Change-Id: I2b36885e2485d23cca199c48b2bd07d5745b00c5
Reviewed-on: https://chromium-review.googlesource.com/c/1346305
Commit-Queue: David Turner <digit@chromium.org>
Reviewed-by: Ehsan Chiniforooshan <chiniforooshan@chromium.org>
Reviewed-by: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611185}

base/trace_event/event_name_filter_unittest.cc

@@ -13,9 +13,8 @@ namespace trace_event {
 
 const TraceEvent& MakeTraceEvent(const char* name) {
   static TraceEvent event;
-  event.Reset();
-  event.Initialize(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0,
-                   0, nullptr, nullptr, nullptr, nullptr, 0);
+  event.Reset(0, TimeTicks(), ThreadTicks(), 'b', nullptr, name, "", 0, 0, 0,
+              nullptr, nullptr, nullptr, nullptr, 0);
   return event;
 }
 

base/trace_event/trace_event_impl.cc

@@ -45,49 +45,24 @@ bool ConvertableToTraceFormat::AppendToProto(ProtoAppender* appender) {
   return false;
 }
 
-TraceEvent::TraceEvent()
-    : duration_(TimeDelta::FromInternalValue(-1)),
-      scope_(trace_event_internal::kGlobalScope),
-      id_(0u),
-      category_group_enabled_(nullptr),
-      name_(nullptr),
-      thread_id_(0),
-      flags_(0),
-      phase_(TRACE_EVENT_PHASE_BEGIN) {
-  for (int i = 0; i < kTraceMaxNumArgs; ++i)
+// See comment for name TraceEvent::scope_ definition.
+static_assert(trace_event_internal::kGlobalScope == nullptr,
+              "Invalid TraceEvent::scope default initializer value");
+
+TraceEvent::TraceEvent() {
+  for (int i = 0; i < kTraceMaxNumArgs; ++i) {
+    arg_values_[i].as_uint = 0u;
     arg_names_[i] = nullptr;
-  memset(arg_values_, 0, sizeof(arg_values_));
+    arg_types_[i] = TRACE_VALUE_TYPE_UINT;
+  }
 }
 
 TraceEvent::~TraceEvent() = default;
 
-void TraceEvent::MoveFrom(std::unique_ptr<TraceEvent> other) {
-  timestamp_ = other->timestamp_;
-  thread_timestamp_ = other->thread_timestamp_;
-  duration_ = other->duration_;
-  scope_ = other->scope_;
-  id_ = other->id_;
-  category_group_enabled_ = other->category_group_enabled_;
-  name_ = other->name_;
-  if (other->flags_ & TRACE_EVENT_FLAG_HAS_PROCESS_ID)
-    process_id_ = other->process_id_;
-  else
-    thread_id_ = other->thread_id_;
-  phase_ = other->phase_;
-  flags_ = other->flags_;
-  parameter_copy_storage_ = std::move(other->parameter_copy_storage_);
+TraceEvent::TraceEvent(TraceEvent&& other) noexcept = default;
+TraceEvent& TraceEvent::operator=(TraceEvent&& other) noexcept = default;
 
-  for (int i = 0; i < kTraceMaxNumArgs; ++i) {
-    arg_names_[i] = other->arg_names_[i];
-    arg_types_[i] = other->arg_types_[i];
-    arg_values_[i] = other->arg_values_[i];
-    convertable_values_[i] = std::move(other->convertable_values_[i]);
-  }
-
-  other->Reset();
-}
-
-void TraceEvent::Initialize(
+TraceEvent::TraceEvent(
     int thread_id,
     TimeTicks timestamp,
     ThreadTicks thread_timestamp,
@@ -102,19 +77,28 @@ void TraceEvent::Initialize(
     const unsigned char* arg_types,
     const unsigned long long* arg_values,
     std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-    unsigned int flags) {
-  timestamp_ = timestamp;
-  thread_timestamp_ = thread_timestamp;
-  duration_ = TimeDelta::FromInternalValue(-1);
-  scope_ = scope;
-  id_ = id;
-  category_group_enabled_ = category_group_enabled;
-  name_ = name;
-  thread_id_ = thread_id;
-  phase_ = phase;
-  flags_ = flags;
-  bind_id_ = bind_id;
+    unsigned int flags)
+    : timestamp_(timestamp),
+      thread_timestamp_(thread_timestamp),
+      scope_(scope),
+      id_(id),
+      category_group_enabled_(category_group_enabled),
+      name_(name),
+      thread_id_(thread_id),
+      flags_(flags),
+      bind_id_(bind_id),
+      phase_(phase) {
+  InitArgs(num_args, arg_names, arg_types, arg_values, convertable_values,
+           flags);
+}
 
+void TraceEvent::InitArgs(
+    int num_args,
+    const char* const* arg_names,
+    const unsigned char* arg_types,
+    const unsigned long long* arg_values,
+    std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+    unsigned int flags) {
   // Clamp num_args since it may have been set by a third_party library.
   num_args = (num_args > kTraceMaxNumArgs) ? kTraceMaxNumArgs : num_args;
   int i = 0;
@@ -139,7 +123,7 @@ void TraceEvent::Initialize(
   bool copy = !!(flags & TRACE_EVENT_FLAG_COPY);
   size_t alloc_size = 0;
   if (copy) {
-    alloc_size += GetAllocLength(name) + GetAllocLength(scope);
+    alloc_size += GetAllocLength(name_) + GetAllocLength(scope_);
     for (i = 0; i < num_args; ++i) {
       alloc_size += GetAllocLength(arg_names_[i]);
       if (arg_types_[i] == TRACE_VALUE_TYPE_STRING)
@@ -182,7 +166,7 @@ void TraceEvent::Initialize(
 }
 
 void TraceEvent::Reset() {
-  // Only reset fields that won't be initialized in Initialize(), or that may
+  // Only reset fields that won't be initialized in Reset(int, ...), or that may
   // hold references to other objects.
   duration_ = TimeDelta::FromInternalValue(-1);
 
@@ -203,6 +187,38 @@ void TraceEvent::Reset() {
     convertable_values_[i].reset();
 }
 
+void TraceEvent::Reset(
+    int thread_id,
+    TimeTicks timestamp,
+    ThreadTicks thread_timestamp,
+    char phase,
+    const unsigned char* category_group_enabled,
+    const char* name,
+    const char* scope,
+    unsigned long long id,
+    unsigned long long bind_id,
+    int num_args,
+    const char* const* arg_names,
+    const unsigned char* arg_types,
+    const unsigned long long* arg_values,
+    std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+    unsigned int flags) {
+  Reset();
+  timestamp_ = timestamp;
+  thread_timestamp_ = thread_timestamp;
+  scope_ = scope;
+  id_ = id;
+  category_group_enabled_ = category_group_enabled;
+  name_ = name;
+  thread_id_ = thread_id;
+  flags_ = flags;
+  bind_id_ = bind_id;
+  phase_ = phase;
+
+  InitArgs(num_args, arg_names, arg_types, arg_values, convertable_values,
+           flags);
+}
+
 void TraceEvent::UpdateDuration(const TimeTicks& now,
                                 const ThreadTicks& thread_now) {
   DCHECK_EQ(duration_.ToInternalValue(), -1);

base/trace_event/trace_event_impl.h

@@ -23,6 +23,7 @@
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_local.h"
+#include "base/trace_event/common/trace_event_common.h"
 #include "base/trace_event/trace_event_memory_overhead.h"
 #include "build/build_config.h"
 
@@ -99,28 +100,55 @@ class BASE_EXPORT TraceEvent {
   };
 
   TraceEvent();
+
+  TraceEvent(int thread_id,
+             TimeTicks timestamp,
+             ThreadTicks thread_timestamp,
+             char phase,
+             const unsigned char* category_group_enabled,
+             const char* name,
+             const char* scope,
+             unsigned long long id,
+             unsigned long long bind_id,
+             int num_args,
+             const char* const* arg_names,
+             const unsigned char* arg_types,
+             const unsigned long long* arg_values,
+             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+             unsigned int flags);
+
   ~TraceEvent();
 
-  void MoveFrom(std::unique_ptr<TraceEvent> other);
-
-  void Initialize(int thread_id,
-                  TimeTicks timestamp,
-                  ThreadTicks thread_timestamp,
-                  char phase,
-                  const unsigned char* category_group_enabled,
-                  const char* name,
-                  const char* scope,
-                  unsigned long long id,
-                  unsigned long long bind_id,
-                  int num_args,
-                  const char* const* arg_names,
-                  const unsigned char* arg_types,
-                  const unsigned long long* arg_values,
-                  std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
-                  unsigned int flags);
+  // Allow move operations.
+  TraceEvent(TraceEvent&&) noexcept;
+  TraceEvent& operator=(TraceEvent&&) noexcept;
 
+  // Reset instance to empty state.
   void Reset();
 
+  // Reset instance to new state. This is equivalent but slightly more
+  // efficient than doing a move assignment, since it avoids creating
+  // temporary copies. I.e. compare these two statements:
+  //
+  //    event = TraceEvent(thread_id, ....);  // Create and destroy temporary.
+  //    event.Reset(thread_id, ...);  // Direct re-initialization.
+  //
+  void Reset(int thread_id,
+             TimeTicks timestamp,
+             ThreadTicks thread_timestamp,
+             char phase,
+             const unsigned char* category_group_enabled,
+             const char* name,
+             const char* scope,
+             unsigned long long id,
+             unsigned long long bind_id,
+             int num_args,
+             const char* const* arg_names,
+             const unsigned char* arg_types,
+             const unsigned long long* arg_values,
+             std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+             unsigned int flags);
+
   void UpdateDuration(const TimeTicks& now, const ThreadTicks& thread_now);
 
   void EstimateTraceMemoryOverhead(TraceEventMemoryOverhead* overhead);
@@ -171,32 +199,43 @@ class BASE_EXPORT TraceEvent {
 #endif
 
  private:
+  void InitArgs(int num_args,
+                const char* const* arg_names,
+                const unsigned char* arg_types,
+                const unsigned long long* arg_values,
+                std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
+                unsigned int flags);
+
   // Note: these are ordered by size (largest first) for optimal packing.
-  TimeTicks timestamp_;
-  ThreadTicks thread_timestamp_;
-  TimeDelta duration_;
-  TimeDelta thread_duration_;
+  TimeTicks timestamp_ = TimeTicks();
+  ThreadTicks thread_timestamp_ = ThreadTicks();
+  TimeDelta duration_ = TimeDelta::FromInternalValue(-1);
+  TimeDelta thread_duration_ = TimeDelta();
   // scope_ and id_ can be used to store phase-specific data.
-  const char* scope_;
-  unsigned long long id_;
+  // The following should be default-initialized to the expression
+  // trace_event_internal::kGlobalScope, which is nullptr, but its definition
+  // cannot be included here due to cyclical header dependencies.
+  // The equivalence is checked with a static_assert() in trace_event_impl.cc.
+  const char* scope_ = nullptr;
+  unsigned long long id_ = 0u;
   TraceValue arg_values_[kTraceMaxNumArgs];
   const char* arg_names_[kTraceMaxNumArgs];
   std::unique_ptr<ConvertableToTraceFormat>
       convertable_values_[kTraceMaxNumArgs];
-  const unsigned char* category_group_enabled_;
-  const char* name_;
+  const unsigned char* category_group_enabled_ = nullptr;
+  const char* name_ = nullptr;
   std::unique_ptr<std::string> parameter_copy_storage_;
   // Depending on TRACE_EVENT_FLAG_HAS_PROCESS_ID the event will have either:
   //  tid: thread_id_, pid: current_process_id (default case).
   //  tid: -1, pid: process_id_ (when flags_ & TRACE_EVENT_FLAG_HAS_PROCESS_ID).
   union {
-    int thread_id_;
+    int thread_id_ = 0;
     int process_id_;
   };
-  unsigned int flags_;
-  unsigned long long bind_id_;
+  unsigned int flags_ = 0;
+  unsigned long long bind_id_ = 0;
   unsigned char arg_types_[kTraceMaxNumArgs];
-  char phase_;
+  char phase_ = TRACE_EVENT_PHASE_BEGIN;
 
   DISALLOW_COPY_AND_ASSIGN(TraceEvent);
 };

base/trace_event/trace_log.cc

@@ -111,21 +111,14 @@ void InitializeMetadataEvent(TraceEvent* trace_event,
   unsigned char arg_type;
   unsigned long long arg_value;
   ::trace_event_internal::SetTraceValue(value, &arg_type, &arg_value);
-  trace_event->Initialize(
-      thread_id,
-      TimeTicks(),
-      ThreadTicks(),
-      TRACE_EVENT_PHASE_METADATA,
-      CategoryRegistry::kCategoryMetadata->state_ptr(),
-      metadata_name,
+
+  trace_event->Reset(
+      thread_id, TimeTicks(), ThreadTicks(), TRACE_EVENT_PHASE_METADATA,
+      CategoryRegistry::kCategoryMetadata->state_ptr(), metadata_name,
       trace_event_internal::kGlobalScope,  // scope
-      trace_event_internal::kNoId,  // id
-      trace_event_internal::kNoId,  // bind_id
-      num_args,
-      &arg_name,
-      &arg_type,
-      &arg_value,
-      nullptr,
+      trace_event_internal::kNoId,         // id
+      trace_event_internal::kNoId,         // bind_id
+      num_args, &arg_name, &arg_type, &arg_value, nullptr,
       TRACE_EVENT_FLAG_NONE);
 }
 
@@ -1291,7 +1284,6 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
       reinterpret_cast<AddTraceEventOverrideCallback>(
           subtle::NoBarrier_Load(&trace_event_override_));
   if (trace_event_override) {
-    TraceEvent new_trace_event;
     // If we have an override in place for events, rather than sending
     // them to the tracelog, we don't have a way of going back and updating
     // the duration of _COMPLETE events. Instead, we emit separate _BEGIN
@@ -1299,7 +1291,7 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
     if (phase == TRACE_EVENT_PHASE_COMPLETE)
       phase = TRACE_EVENT_PHASE_BEGIN;
 
-    new_trace_event.Initialize(thread_id, offset_event_timestamp, thread_now,
+    TraceEvent new_trace_event(thread_id, offset_event_timestamp, thread_now,
                                phase, category_group_enabled, name, scope, id,
                                bind_id, num_args, arg_names, arg_types,
                                arg_values, convertable_values, flags);
@@ -1314,11 +1306,10 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
   std::unique_ptr<TraceEvent> filtered_trace_event;
   bool disabled_by_filters = false;
   if (*category_group_enabled & TraceCategory::ENABLED_FOR_FILTERING) {
-    std::unique_ptr<TraceEvent> new_trace_event(new TraceEvent);
-    new_trace_event->Initialize(thread_id, offset_event_timestamp, thread_now,
-                                phase, category_group_enabled, name, scope, id,
-                                bind_id, num_args, arg_names, arg_types,
-                                arg_values, convertable_values, flags);
+    auto new_trace_event = std::make_unique<TraceEvent>(
+        thread_id, offset_event_timestamp, thread_now, phase,
+        category_group_enabled, name, scope, id, bind_id, num_args, arg_names,
+        arg_types, arg_values, convertable_values, flags);
 
     disabled_by_filters = true;
     ForEachCategoryFilter(
@@ -1347,12 +1338,12 @@ TraceEventHandle TraceLog::AddTraceEventWithThreadIdAndTimestamp(
 
     if (trace_event) {
       if (filtered_trace_event) {
-        trace_event->MoveFrom(std::move(filtered_trace_event));
+        *trace_event = std::move(*filtered_trace_event);
       } else {
-        trace_event->Initialize(thread_id, offset_event_timestamp, thread_now,
-                                phase, category_group_enabled, name, scope, id,
-                                bind_id, num_args, arg_names, arg_types,
-                                arg_values, convertable_values, flags);
+        trace_event->Reset(thread_id, offset_event_timestamp, thread_now, phase,
+                           category_group_enabled, name, scope, id, bind_id,
+                           num_args, arg_names, arg_types, arg_values,
+                           convertable_values, flags);
       }
 
 #if defined(OS_ANDROID)
@@ -1383,12 +1374,11 @@ void TraceLog::AddMetadataEvent(
     std::unique_ptr<ConvertableToTraceFormat>* convertable_values,
     unsigned int flags) {
   HEAP_PROFILER_SCOPED_IGNORE;
-  std::unique_ptr<TraceEvent> trace_event(new TraceEvent);
   int thread_id = static_cast<int>(base::PlatformThread::CurrentId());
   ThreadTicks thread_now = ThreadNow();
   TimeTicks now = OffsetNow();
   AutoLock lock(lock_);
-  trace_event->Initialize(
+  auto trace_event = std::make_unique<TraceEvent>(
       thread_id, now, thread_now, TRACE_EVENT_PHASE_METADATA,
       category_group_enabled, name,
       trace_event_internal::kGlobalScope,  // scope
@@ -1504,8 +1494,7 @@ void TraceLog::UpdateTraceEventDurationExplicit(
     // we don't have way of updating the prior event so we'll emit a
     // separate _END event instead.
     if (trace_event_override) {
-      TraceEvent new_trace_event;
-      new_trace_event.Initialize(
+      TraceEvent new_trace_event(
           static_cast<int>(base::PlatformThread::CurrentId()), now, thread_now,
           TRACE_EVENT_PHASE_END, category_group_enabled, name,
           trace_event_internal::kGlobalScope,
@@ -1591,7 +1580,7 @@ void TraceLog::AddMetadataEventsWhileLocked() {
     while (!metadata_events_.empty()) {
       TraceEvent* event =
           AddEventToThreadSharedChunkWhileLocked(nullptr, false);
-      event->MoveFrom(std::move(metadata_events_.back()));
+      *event = std::move(*metadata_events_.back());
       metadata_events_.pop_back();
     }
   }