Stop holding onto TimelineEvents

runtime/vm/timeline.cc

Index: runtime/vm/timeline.cc
diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc
index 2fd5167998b8ee85f82238945e9e635f9a5d60fc..1c30b053fd8840dc127faebaa7693a030e1934d7 100644
--- a/runtime/vm/timeline.cc
+++ b/runtime/vm/timeline.cc
@@ -278,6 +278,9 @@ void TimelineEvent::SetNumArguments(intptr_t length) {
   // Cannot call this twice.
   ASSERT(arguments_ == NULL);
   ASSERT(arguments_length_ == 0);
+  if (length == 0) {
+    return;
+  }
   arguments_length_ = length;
   arguments_ = reinterpret_cast<TimelineEventArgument*>(
       calloc(sizeof(TimelineEventArgument), length));
@@ -318,6 +321,21 @@ void TimelineEvent::CopyArgument(intptr_t i,
 }
 
 
+void TimelineEvent::StealArguments(intptr_t arguments_length,
+                                   TimelineEventArgument* arguments) {
+  if (arguments_length == 0) {
+    return;
+  }
+  SetNumArguments(arguments_length);
+  for (intptr_t i = 0; i < arguments_length; i++) {
+    SetArgument(i, arguments[i].name, arguments[i].value);
+    // Steal.
+    arguments[i].name = NULL;
+    arguments[i].value = NULL;
+  }

[turnidge, 2015/10/08 17:59:19]

Perhaps you could steal the whole arguments array and skip the loop?

arguments_length_ = arguments_length;
arguments_ = arguments;

[Cutch, 2015/10/08 20:08:10]

Done.

+}
+
+
 void TimelineEvent::Complete() {
   TimelineEventRecorder* recorder = Timeline::recorder();
   if (recorder != NULL) {
@@ -467,10 +485,94 @@ TimelineEvent* TimelineStream::StartEvent() {
 }
 
 
+void TimelineDurationScope::Init() {
+  ASSERT(enabled_ == false);
+  ASSERT(label_ != NULL);
+  ASSERT(stream_ != NULL);
+  if (!stream_->Enabled()) {
+    // Stream is not enabled, do nothing.
+    return;
+  }
+  TimelineEvent* event = stream_->StartEvent();
+  if (event == NULL) {
+    return;
+  }
+  enabled_ = true;
+  event->Begin(label_);
+  event->Complete();
+}
+
+void TimelineDurationScope::FreeArguments() {
+  if (arguments_ == NULL) {
+    return;
+  }
+  for (intptr_t i = 0; i < arguments_length_; i++) {
+    free(arguments_[i].value);
+  }
+  free(arguments_);
+  arguments_ = NULL;
+  arguments_length_ = 0;
+}
+
+
+TimelineDurationScope::~TimelineDurationScope() {
+  if (!enabled_) {
+    FreeArguments();
+    return;
+  }
+  TimelineEvent* event = stream_->StartEvent();
+  if (event == NULL) {
+    FreeArguments();
+    return;
+  }
+  event->End(label_);
+  event->StealArguments(arguments_length_, arguments_);

[turnidge, 2015/10/08 17:59:19]

If StealArguments steals the whole array, follow this by:

arguments_length_ = 0;
arguments_ = NULL;

and remove the call to FreeArguments.

[Cutch, 2015/10/08 20:08:10]

Done.

+  event->Complete();
+  FreeArguments();
+}
+
+
+void TimelineDurationScope::SetNumArguments(intptr_t length) {
+  if (!enabled()) {
+    return;
+  }
+  ASSERT(arguments_ == NULL);
+  ASSERT(arguments_length_ == 0);
+  arguments_length_ = length;

[turnidge, 2015/10/08 17:59:19]

You could choose to guard the allocation of arguments_ here...

if (arguments_length_ > 0) {
  calloc
}

[Cutch, 2015/10/08 20:08:10]

Done.

+  arguments_ = reinterpret_cast<TimelineEventArgument*>(
+      calloc(sizeof(TimelineEventArgument), length));
+}
+
+
+// |name| must be a compile time constant. Takes ownership of |argumentp|.
+void TimelineDurationScope::SetArgument(intptr_t i,
+                                        const char* name,
+                                        char* argument) {
+  if (!enabled()) {
+    return;
+  }
+  ASSERT(i >= 0);
+  ASSERT(i < arguments_length_);
+  arguments_[i].name = name;
+  arguments_[i].value = argument;
+}
+
+
+// |name| must be a compile time constant. Copies |argument|.
+void TimelineDurationScope::CopyArgument(intptr_t i,
+                                         const char* name,
+                                         const char* argument) {
+  if (!enabled()) {
+    return;
+  }
+  SetArgument(i, name, strdup(argument));
+}
+
+
 void TimelineDurationScope::FormatArgument(intptr_t i,
                                            const char* name,
                                            const char* fmt, ...) {
-  if (event_ == NULL) {
+  if (!enabled()) {
     return;
   }
   va_list args;
@@ -484,7 +586,7 @@ void TimelineDurationScope::FormatArgument(intptr_t i,
   OS::VSNPrint(buffer, (len + 1), fmt, args2);
   va_end(args2);
 
-  event_->SetArgument(i, name, buffer);
+  SetArgument(i, name, buffer);
 }
 
 
@@ -545,13 +647,12 @@ TimelineEvent* TimelineEventRecorder::ThreadBlockStartEvent() {
   // Grab the current thread.
   Thread* thread = Thread::Current();
   ASSERT(thread != NULL);
-  // We are accessing the thread's timeline block- so take the lock.
-  MutexLocker ml(thread->timeline_block_lock());
-
-  if (thread->isolate() == NULL) {
-    // Non-isolate thread case. This should be infrequent.
-    return GlobalBlockStartEvent();
-  }
+  ASSERT(thread->isolate() != NULL);
+  Mutex* thread_block_lock = thread->timeline_block_lock();
+  ASSERT(thread_block_lock != NULL);
+  // We are accessing the thread's timeline block- so take the lock here.
+  // This lock will be held until the call to |CompleteEvent| is made.
+  thread_block_lock->Lock();
 
   TimelineEventBlock* thread_block = thread->timeline_block();
 
@@ -570,16 +671,20 @@ TimelineEvent* TimelineEventRecorder::ThreadBlockStartEvent() {
     thread->set_timeline_block(thread_block);
   }
   if (thread_block != NULL) {
+    // NOTE: We are exiting this function with the thread's block lock held.
     ASSERT(!thread_block->IsFull());
     return thread_block->StartEvent();
   }
+  // Drop lock here as no event is being handed out.
+  thread_block_lock->Unlock();
   return NULL;
 }
 
 
-
 TimelineEvent* TimelineEventRecorder::GlobalBlockStartEvent() {
-  MutexLocker ml(&lock_);
+  // Take recorder lock. This lock will be held until the call to
+  // |CompleteEvent| is made.
+  lock_.Lock();
   if (FLAG_trace_timeline) {
     OS::Print("GlobalBlockStartEvent in block %p for thread %" Px "\n",
               global_block_, OSThread::CurrentCurrentThreadIdAsIntPtr());
@@ -595,13 +700,36 @@ TimelineEvent* TimelineEventRecorder::GlobalBlockStartEvent() {
     ASSERT(global_block_ != NULL);
   }
   if (global_block_ != NULL) {
+    // NOTE: We are exiting this function with the recorder's lock held.
     ASSERT(!global_block_->IsFull());
     return global_block_->StartEvent();
   }
+  // Drop lock here as no event is being handed out.
+  lock_.Unlock();
   return NULL;
 }
 
 
+void TimelineEventRecorder::ThreadBlockCompleteEvent(TimelineEvent* event) {
+  // Grab the current thread.
+  Thread* thread = Thread::Current();
+  ASSERT(thread != NULL);
+  ASSERT(thread->isolate() != NULL);
+  // This event came from the isolate's thread local block. Unlock the
+  // thread's block lock.
+  Mutex* thread_block_lock = thread->timeline_block_lock();
+  ASSERT(thread_block_lock != NULL);
+  thread_block_lock->Unlock();
+}
+
+
+void TimelineEventRecorder::GlobalBlockCompleteEvent(TimelineEvent* event) {
+  // This event came from the global block, unlock the recorder's lock now
+  // that the event is filled.
+  lock_.Unlock();
+}
+
+
 // Trims the ']' character.
 static void TrimOutput(char* output,
                        intptr_t* output_length) {
@@ -869,12 +997,28 @@ intptr_t TimelineEventRingRecorder::FindOldestBlockIndex() const {
 
 
 TimelineEvent* TimelineEventRingRecorder::StartEvent() {
+  // Grab the current thread.
+  Thread* thread = Thread::Current();
+  ASSERT(thread != NULL);
+  if (thread->isolate() == NULL) {
+    // Non-isolate thread case. This should be infrequent.
+    return GlobalBlockStartEvent();
+  }
   return ThreadBlockStartEvent();
 }
 
 
 void TimelineEventRingRecorder::CompleteEvent(TimelineEvent* event) {
-  // no-op.
+  if (event == NULL) {
+    return;
+  }
+  Thread* thread = Thread::Current();
+  ASSERT(thread != NULL);
+  if (thread->isolate() == NULL) {
+    GlobalBlockCompleteEvent(event);
+  } else {
+    ThreadBlockCompleteEvent(event);
+  }
 }
 
 
@@ -996,12 +1140,28 @@ TimelineEventBlock* TimelineEventEndlessRecorder::GetHeadBlockLocked() {
 
 
 TimelineEvent* TimelineEventEndlessRecorder::StartEvent() {
+  // Grab the current thread.
+  Thread* thread = Thread::Current();
+  ASSERT(thread != NULL);
+  if (thread->isolate() == NULL) {
+    // Non-isolate thread case. This should be infrequent.
+    return GlobalBlockStartEvent();
+  }
   return ThreadBlockStartEvent();
 }
 
 
 void TimelineEventEndlessRecorder::CompleteEvent(TimelineEvent* event) {
-  // no-op.
+  if (event == NULL) {
+    return;
+  }
+  Thread* thread = Thread::Current();
+  ASSERT(thread != NULL);
+  if (thread->isolate() == NULL) {
+    GlobalBlockCompleteEvent(event);
+  } else {
+    ThreadBlockCompleteEvent(event);
+  }
 }