Simplify timeline backend

runtime/vm/timeline.cc

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include <cstdlib>
 
 #include "vm/atomic.h"
 #include "vm/isolate.h"
 #include "vm/json_stream.h"
 #include "vm/lockers.h"
@@ skipping 23 matching lines @@
 // synchronizing with other threads in the system. Even though the |Thread| owns
 // the |TimelineEventBlock| the block may need to be reclaimed by the reporting
 // system. To support that, a |Thread| must hold its |timeline_block_lock_|
 // when operating on the |TimelineEventBlock|. This lock will only ever be
 // busy if blocks are being reclaimed by the reporting system.
 //
 // Reporting:
 // When requested, the timeline is serialized in the trace-event format
 // (https://goo.gl/hDZw5M). The request can be for a VM-wide timeline or an
 // isolate specific timeline. In both cases it may be that a thread has
-// a |TimelineEventBlock| cached in TLS. In order to report a complete timeline
-// the cached |TimelineEventBlock|s need to be reclaimed.
+// a |TimelineEventBlock| cached in TLS partially filled with events. In order

[turnidge, 2015/10/16 19:40:20]

Is it really cached in TLS?  Seems out of date.

[Cutch, 2015/10/16 19:43:52]

Done.

+// to report a complete timeline the cached |TimelineEventBlock|s need to be
+// reclaimed.
 //
-// Reclaiming open |TimelineEventBlock|s for an isolate:
+// Reclaiming open |TimelineEventBlock|s from threads:
 //
-// Cached |TimelineEventBlock|s can be in two places:
-// 1) In a |Thread| (Thread currently in an |Isolate|)
-// 2) In a |Thread::State| (Thread not currently in an |Isolate|).
+// Each |Thread| can have one |TimelineEventBlock| cached in it.
 //
-// As a |Thread| enters and exits an |Isolate|, a |TimelineEventBlock|
-// will move between (1) and (2).
-//
-// The first case occurs for |Thread|s that are currently running inside an
-// isolate. The second case occurs for |Thread|s that are not currently
-// running inside an isolate.
-//
-// To reclaim the first case, we take the |Thread|'s |timeline_block_lock_|
-// and reclaim the cached block.
-//
-// To reclaim the second case, we can take the |ThreadRegistry| lock and
-// reclaim these blocks.
-//
-// |Timeline::ReclaimIsolateBlocks| and |Timeline::ReclaimAllBlocks| are
-// the two utility methods used to reclaim blocks before reporting.
+// To reclaim blocks, we iterate over all threads and remove the cached
+// |TimelineEventBlock| from each thread. This is safe because we hold the
+// |Thread|'s |timeline_block_lock_| meaning the block can't be being modified.
 //
 // Locking notes:
 // The following locks are used by the timeline system:
 // - |TimelineEventRecorder::lock_| This lock is held whenever a
 // |TimelineEventBlock| is being requested or reclaimed.
 // - |Thread::timeline_block_lock_| This lock is held whenever a |Thread|'s
 // cached block is being operated on.
-// - |ThreadRegistry::monitor_| This lock protects the cached block for
-// unscheduled threads of an isolate.
-// - |Isolate::isolates_list_monitor_| This lock protects the list of
-// isolates in the system.
+// - |Thread::thread_list_lock_| This lock is held when iterating over
+// |Thread|s.
 //
 // Locks must always be taken in the following order:
-// |Isolate::isolates_list_monitor_|
-//   |ThreadRegistry::monitor_|
+//   |Thread::thread_list_lock_|
 //     |Thread::timeline_block_lock_|
 //       |TimelineEventRecorder::lock_|
 //
 
 void Timeline::InitOnce() {
   ASSERT(recorder_ == NULL);
   // Default to ring recorder being enabled.
   const bool use_ring_recorder = true;
   // Some flags require that we use the endless recorder.
   const bool use_endless_recorder =
@@ skipping 35 matching lines @@
   return (FLAG_timeline_dir != NULL) || FLAG_timing;
 }
 
 
 TimelineStream* Timeline::GetVMStream() {
   ASSERT(vm_stream_ != NULL);
   return vm_stream_;
 }
 
 
-void Timeline::ReclaimIsolateBlocks() {
-  ReclaimBlocksForIsolate(Isolate::Current());
+void Timeline::ReclaimCachedBlocksFromThreads() {
+  TimelineEventRecorder* recorder = Timeline::recorder();
+  if (recorder == NULL) {
+    return;
+  }
+
+  // Iterate over threads.
+  ThreadIterator it;
+  while (it.HasNext()) {
+    Thread* thread = it.Next();
+    MutexLocker ml(thread->timeline_block_lock());
+    // Grab block and clear it.
+    TimelineEventBlock* block = thread->timeline_block();
+    thread->set_timeline_block(NULL);

[turnidge, 2015/10/16 19:40:20]

Should we release the timeline_block_lock before we call FinishBlock?  Or is it
irrelevant?

[Cutch, 2015/10/16 19:43:52]

For now I'm keeping this consistent with the documented lock ordering. In the
future I would like to do this. I've added a TODO for myself to circle back.

+    recorder->FinishBlock(block);
+  }
 }
 
 
-class ReclaimBlocksIsolateVisitor : public IsolateVisitor {
- public:
-  ReclaimBlocksIsolateVisitor() {}
-
-  virtual void VisitIsolate(Isolate* isolate) {
-    Timeline::ReclaimBlocksForIsolate(isolate);
-  }
-
- private:
-};
-
-
-void Timeline::ReclaimAllBlocks() {
-  if (recorder() == NULL) {
-    return;
-  }
-  // Reclaim all blocks cached for all isolates.
-  ReclaimBlocksIsolateVisitor visitor;
-  Isolate::VisitIsolates(&visitor);
-  // Reclaim the global VM block.
-  recorder()->ReclaimGlobalBlock();
-}
-
-
-void Timeline::ReclaimBlocksForIsolate(Isolate* isolate) {
-  if (recorder() == NULL) {
-    return;
-  }
-  ASSERT(isolate != NULL);
-  isolate->ReclaimTimelineBlocks();
-}
-
-
 TimelineEventRecorder* Timeline::recorder_ = NULL;
 TimelineStream* Timeline::vm_stream_ = NULL;
 
 #define ISOLATE_TIMELINE_STREAM_DEFINE_FLAG(name, enabled_by_default)          \
   bool Timeline::stream_##name##_enabled_ = false;
   ISOLATE_TIMELINE_STREAM_LIST(ISOLATE_TIMELINE_STREAM_DEFINE_FLAG)
 #undef ISOLATE_TIMELINE_STREAM_DEFINE_FLAG
 
 TimelineEvent::TimelineEvent()
     : timestamp0_(0),
@@ skipping 463 matching lines @@
 
 void DartTimelineEvent::Init(Isolate* isolate, const char* event) {
   ASSERT(isolate_ == NULL);
   ASSERT(event != NULL);
   isolate_ = isolate;
   event_as_json_ = strdup(event);
 }
 
 
 TimelineEventRecorder::TimelineEventRecorder()
-    : global_block_(NULL),
-      async_id_(0) {
+    : async_id_(0) {
 }
 
 
 void TimelineEventRecorder::PrintJSONMeta(JSONArray* events) const {
 }
 
 
 TimelineEvent* TimelineEventRecorder::ThreadBlockStartEvent() {
   // Grab the current thread.
   Thread* thread = Thread::Current();
@@ skipping 18 matching lines @@
   } else if (thread_block == NULL) {
     MutexLocker ml(&lock_);
     // Thread has no block. Attempt to allocate one.
     thread_block = GetNewBlockLocked(thread->isolate());
     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());
     TimelineEvent* event = thread_block->StartEvent();
-    if (event != NULL) {
-      event->set_global_block(false);
-    }
     return event;
   }
   // Drop lock here as no event is being handed out.
   thread_block_lock->Unlock();
   return NULL;
 }
 
 
-TimelineEvent* TimelineEventRecorder::GlobalBlockStartEvent() {
-  // 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());
-  }
-  if ((global_block_ != NULL) && global_block_->IsFull()) {
-    // Global block is full.
-    global_block_->Finish();
-    global_block_ = NULL;
-  }
-  if (global_block_ == NULL) {
-    // Allocate a new block.
-    global_block_ = GetNewBlockLocked(NULL);
-    ASSERT(global_block_ != NULL);
-  }
-  if (global_block_ != NULL) {
-    // NOTE: We are exiting this function with the recorder's lock held.
-    ASSERT(!global_block_->IsFull());
-    TimelineEvent* event = global_block_->StartEvent();
-    if (event != NULL) {
-      event->set_global_block(true);
-    }
-    return event;
-  }
-  // Drop lock here as no event is being handed out.
-  lock_.Unlock();
-  return NULL;
-}
-
-
 void TimelineEventRecorder::ThreadBlockCompleteEvent(TimelineEvent* event) {
   if (event == NULL) {
     return;
   }
-  ASSERT(!event->global_block());
   // 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.
+  // 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) {
-  if (event == NULL) {
-    return;
-  }
-  ASSERT(event->global_block());
-  // 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) {
   ASSERT(output != NULL);
   ASSERT(output_length != NULL);
   ASSERT(*output_length >= 2);
   // We expect the first character to be the opening of an array.
   ASSERT(output[0] == '[');
   // We expect the last character to be the closing of an array.
   ASSERT(output[*output_length - 1] == ']');
   // Skip the ].
   *output_length -= 1;
 }
 
 
 void TimelineEventRecorder::WriteTo(const char* directory) {
   Dart_FileOpenCallback file_open = Isolate::file_open_callback();
   Dart_FileWriteCallback file_write = Isolate::file_write_callback();
   Dart_FileCloseCallback file_close = Isolate::file_close_callback();
   if ((file_open == NULL) || (file_write == NULL) || (file_close == NULL)) {
     return;
   }
   Thread* T = Thread::Current();
   StackZone zone(T);
 
-  Timeline::ReclaimAllBlocks();
+  Timeline::ReclaimCachedBlocksFromThreads();
 
   intptr_t pid = OS::ProcessId();
   char* filename = OS::SCreate(NULL,
       "%s/dart-timeline-%" Pd ".json", directory, pid);
   void* file = (*file_open)(filename, true);
   if (file == NULL) {
     OS::Print("Failed to write timeline file: %s\n", filename);
     free(filename);
     return;
   }
@@ skipping 31 matching lines @@
 
   // Write out the ']' character.
   const char* array_close = "]";
   (*file_write)(array_close, 1, file);
   (*file_close)(file);
 
   return;
 }
 
 
-void TimelineEventRecorder::ReclaimGlobalBlock() {
-  MutexLocker ml(&lock_);
-  if (global_block_ != NULL) {
-    global_block_->Finish();
-    global_block_ = NULL;
-  }
-}
-
-
 int64_t TimelineEventRecorder::GetNextAsyncId() {
   // TODO(johnmccutchan): Gracefully handle wrap around.
   uint32_t next = static_cast<uint32_t>(
       AtomicOperations::FetchAndIncrement(&async_id_));
   return static_cast<int64_t>(next);
 }
 
 
 void TimelineEventRecorder::FinishBlock(TimelineEventBlock* block) {
   if (block == NULL) {
@@ skipping 164 matching lines @@
     }
   }
   return earliest_index;
 }
 
 
 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) {
   if (event == NULL) {
     return;
   }
-  if (event->global_block()) {
-    GlobalBlockCompleteEvent(event);
-  } else {
-    ThreadBlockCompleteEvent(event);
-  }
+  ThreadBlockCompleteEvent(event);
 }
 
 
 TimelineEventStreamingRecorder::TimelineEventStreamingRecorder() {
 }
 
 
 TimelineEventStreamingRecorder::~TimelineEventStreamingRecorder() {
 }
 
@@ skipping 104 matching lines @@
 
 TimelineEventBlock* TimelineEventEndlessRecorder::GetHeadBlockLocked() {
   return head_;
 }
 
 
 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) {
   if (event == NULL) {
     return;
   }
-  if (event->global_block()) {
-    GlobalBlockCompleteEvent(event);
-  } else {
-    ThreadBlockCompleteEvent(event);
-  }
+  ThreadBlockCompleteEvent(event);
 }
 
 
 TimelineEventBlock* TimelineEventEndlessRecorder::GetNewBlockLocked(
     Isolate* isolate) {
   TimelineEventBlock* block = new TimelineEventBlock(block_index_++);
   block->set_next(head_);
   block->Open(isolate);
   head_ = block;
   if (FLAG_trace_timeline) {
@@ skipping 258 matching lines @@
       // If an isolate was specified, skip events from other isolates.
       continue;
     }
     ASSERT(event->event_as_json() != NULL);
     result = zone->ConcatStrings(result, event->event_as_json());
   }
   return result;
 }
 
 }  // namespace dart