Make TimelineEventBlocks reclaimable

runtime/vm/timeline.cc

Index: runtime/vm/timeline.cc
diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc
index 6b03d99fa5101720ff17a27b9c990ceb3e8e861e..24a7e787689d79e66acf07153581375ebbf073fd 100644
--- a/runtime/vm/timeline.cc
+++ b/runtime/vm/timeline.cc
@@ -26,6 +26,64 @@ DEFINE_FLAG(charp, timeline_dir, NULL,
             "Enable all timeline trace streams and output VM global trace "
             "into specified directory.");
 
+// Implementation notes:
+//
+// Writing events:
+// |TimelineEvent|s are written into |TimelineEventBlock|s. Each |Thread| caches
+// a |TimelineEventBlock| in TLS so that it can write events without
+// 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.
+//
+// Reclaiming open |TimelineEventBlock|s for an isolate:
+//
+// 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|).
+//
+// 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.
+//
+// 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.
+//
+// Locks must always be taken in the following order:
+// |Isolate::isolates_list_monitor_|
+//   |ThreadRegistry::monitor_|
+//     |Thread::timeline_block_lock_|
+//       |TimelineEventRecorder::lock_|
+//
+
 void Timeline::InitOnce() {
   ASSERT(recorder_ == NULL);
   // Default to ring recorder being enabled.
@@ -77,6 +135,49 @@ TimelineStream* Timeline::GetVMStream() {
 }
 
 
+void Timeline::ReclaimIsolateBlocks() {
+  if (recorder() == NULL) {
+    return;
+  }

[turnidge, 2015/09/24 17:05:52]

This check is redundant (also in callee).

[Cutch, 2015/09/24 18:14:16]

Done.

+  ReclaimBlocksForIsolate(Isolate::Current());
+}
+
+
+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;
+  }
+  if (isolate == NULL) {
+    return;
+  }

[turnidge, 2015/09/24 17:05:52]

Consider an ASSERT for isolate != NULL?

[Cutch, 2015/09/24 18:14:16]

Done.

+  isolate->ReclaimTimelineBlocks();
+}
+
+
 TimelineEventRecorder* Timeline::recorder_ = NULL;
 TimelineStream* Timeline::vm_stream_ = NULL;
 
@@ -394,9 +495,11 @@ void TimelineEventRecorder::PrintJSONMeta(JSONArray* events) const {
 
 
 TimelineEvent* TimelineEventRecorder::ThreadBlockStartEvent() {
-  // Grab the thread's timeline event block.
+  // 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.
@@ -460,7 +563,7 @@ void TimelineEventRecorder::WriteTo(const char* directory) {
     return;
   }
 
-  FinishGlobalBlock();
+  Timeline::ReclaimAllBlocks();
 
   JSONStream js;
   TimelineEventFilter filter;
@@ -481,7 +584,7 @@ void TimelineEventRecorder::WriteTo(const char* directory) {
 }
 
 
-void TimelineEventRecorder::FinishGlobalBlock() {
+void TimelineEventRecorder::ReclaimGlobalBlock() {
   MutexLocker ml(&lock_);
   if (global_block_ != NULL) {
     global_block_->Finish();