Revert "[VM/Timeline] Improve handling of track metadata"

This reverts commit 3787601.

Reason for revert: Introduces a crash that's blocking the Dart->Flutter roller: flutter/flutter#120012

Original change's description:
> [VM/Timeline] Improve handling of track metadata
>
> The track information that we send over the service is currently
> incomplete. Since we only iterate over the threads that exist at the
> time when getVMTimeline is called, we're missing information about any
> threads that were joined before that. This CL fixes this issue.
>
> TEST=CI, ASAN, and I manually checked that more track names are
> populated when the changes in this CL are applied
>
> Change-Id: Ic4edc9910884c3f4743dc0ca65aa7d2a695ff09d
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/280058
> Reviewed-by: Ben Konyi <[email protected]>
> Commit-Queue: Derek Xu <[email protected]>

[email protected],[email protected],[email protected]

Change-Id: I920c8d439a3b253c331dd730b637dbfeaad62157
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/281000
Commit-Queue: Derek Xu <[email protected]>
Reviewed-by: Derek Xu <[email protected]>
Auto-Submit: Brandon DeRosier <[email protected]>

Author: bdero

runtime/vm/timeline.cc

@@ -11,10 +11,8 @@
 #include <fcntl.h>
 
 #include <cstdlib>
-#include <utility>
 
 #include "platform/atomic.h"
-#include "platform/hashmap.h"
 #include "vm/isolate.h"
 #include "vm/json_stream.h"
 #include "vm/lockers.h"
@@ -631,16 +629,6 @@ void TimelineEvent::Init(EventType event_type, const char* label) {
   OSThread* os_thread = OSThread::Current();
   ASSERT(os_thread != NULL);
   thread_ = os_thread->trace_id();
-  if (FLAG_timeline_recorder != nullptr &&
-      // There is no way to retrieve track metadata when a callback or systrace
-      // recorder is in use, so we don't need to call
-      // |AddTrackMetadataBasedOnThread()| in these cases.
-      strcmp("callback", FLAG_timeline_recorder) != 0 &&
-      strcmp("systrace", FLAG_timeline_recorder) != 0) {
-    RecorderLockScope rl;
-    TimelineEventRecorder* recorder = Timeline::recorder();
-    recorder->AddTrackMetadataBasedOnThread(*os_thread);
-  }
   auto thread = Thread::Current();
   auto isolate = thread != nullptr ? thread->isolate() : nullptr;
   auto isolate_group = thread != nullptr ? thread->isolate_group() : nullptr;
@@ -853,27 +841,6 @@ const char* TimelineEvent::GetFormattedIsolateGroupId() const {
   return formatted_isolate_group_id;
 }
 
-TimelineTrackMetadata::TimelineTrackMetadata(
-    intptr_t pid,
-    intptr_t tid,
-    Utils::CStringUniquePtr&& track_name)
-    : pid_(pid), tid_(tid), track_name_(std::move(track_name)) {}
-
-#if !defined(PRODUCT)
-void TimelineTrackMetadata::PrintJSON(const JSONArray& jsarr_events) const {
-  JSONObject jsobj(&jsarr_events);
-  jsobj.AddProperty("name", "thread_name");
-  jsobj.AddProperty("ph", "M");
-  jsobj.AddProperty("pid", pid());
-  jsobj.AddProperty("tid", tid());
-  {
-    JSONObject jsobj_args(&jsobj, "args");
-    jsobj_args.AddPropertyF("name", "%s (%" Pd ")", track_name(), tid());
-    jsobj_args.AddProperty("mode", "basic");
-  }
-}
-#endif  // !defined(PRODUCT)
-
 TimelineStream::TimelineStream(const char* name,
                                const char* fuchsia_name,
                                bool has_static_labels,
@@ -1067,28 +1034,30 @@ IsolateTimelineEventFilter::IsolateTimelineEventFilter(
       isolate_id_(isolate_id) {}
 
 TimelineEventRecorder::TimelineEventRecorder()
-    : time_low_micros_(0),
-      time_high_micros_(0),
-      track_uuid_to_track_metadata_(
-          &SimpleHashMap::SamePointerValue,
-          TimelineEventRecorder::kTrackUuidToTrackMetadataInitialCapacity) {}
-
-TimelineEventRecorder::~TimelineEventRecorder() {
-  for (SimpleHashMap::Entry* entry = track_uuid_to_track_metadata_.Start();
-       entry != nullptr; entry = track_uuid_to_track_metadata_.Next(entry)) {
-    TimelineTrackMetadata* value =
-        static_cast<TimelineTrackMetadata*>(entry->value);
-    delete value;
-  }
-}
+    : time_low_micros_(0), time_high_micros_(0) {}
 
 #ifndef PRODUCT
-void TimelineEventRecorder::PrintJSONMeta(const JSONArray& jsarr_events) const {
-  for (SimpleHashMap::Entry* entry = track_uuid_to_track_metadata_.Start();
-       entry != nullptr; entry = track_uuid_to_track_metadata_.Next(entry)) {
-    TimelineTrackMetadata* value =
-        static_cast<TimelineTrackMetadata*>(entry->value);
-    value->PrintJSON(jsarr_events);
+void TimelineEventRecorder::PrintJSONMeta(JSONArray* events) const {
+  OSThreadIterator it;
+  while (it.HasNext()) {
+    OSThread* thread = it.Next();
+    const char* thread_name = thread->name();
+    if (thread_name == NULL) {
+      // Only emit a thread name if one was set.
+      continue;
+    }
+    JSONObject obj(events);
+    int64_t pid = OS::ProcessId();
+    int64_t tid = OSThread::ThreadIdToIntPtr(thread->trace_id());
+    obj.AddProperty("name", "thread_name");
+    obj.AddProperty("ph", "M");
+    obj.AddProperty64("pid", pid);
+    obj.AddProperty64("tid", tid);
+    {
+      JSONObject args(&obj, "args");
+      args.AddPropertyF("name", "%s (%" Pd64 ")", thread_name, tid);
+      args.AddProperty("mode", "basic");
+    }
   }
 }
 #endif
@@ -1240,25 +1209,6 @@ TimelineEventBlock* TimelineEventRecorder::GetNewBlock() {
   return GetNewBlockLocked();
 }
 
-void TimelineEventRecorder::AddTrackMetadataBasedOnThread(
-    const OSThread& thread) {
-  intptr_t pid = OS::ProcessId();
-  intptr_t tid = OSThread::ThreadIdToIntPtr(thread.trace_id());
-  const char* thread_name = thread.name();
-
-  void* key = reinterpret_cast<void*>(tid);
-  const intptr_t hash = Utils::WordHash(tid);
-  SimpleHashMap::Entry* entry =
-      track_uuid_to_track_metadata_.Lookup(key, hash, true);
-  if (entry->value == nullptr) {
-    TimelineTrackMetadata* metadata = new TimelineTrackMetadata(
-        pid, tid,
-        Utils::CreateCStringUniquePtr(
-            Utils::StrDup(thread_name == nullptr ? "" : thread_name)));
-    entry->value = metadata;
-  }
-}
-
 TimelineEventFixedBufferRecorder::TimelineEventFixedBufferRecorder(
     intptr_t capacity)
     : memory_(NULL),
@@ -1332,7 +1282,7 @@ void TimelineEventFixedBufferRecorder::PrintJSON(JSONStream* js,
   topLevel.AddProperty("type", "Timeline");
   {
     JSONArray events(&topLevel, "traceEvents");
-    PrintJSONMeta(events);
+    PrintJSONMeta(&events);
     PrintJSONEvents(&events, filter);
   }
   topLevel.AddPropertyTimeMicros("timeOriginMicros", TimeOriginMicros());
@@ -1343,7 +1293,7 @@ void TimelineEventFixedBufferRecorder::PrintTraceEvent(
     JSONStream* js,
     TimelineEventFilter* filter) {
   JSONArray events(js);
-  PrintJSONMeta(events);
+  PrintJSONMeta(&events);
   PrintJSONEvents(&events, filter);
 }
 #endif
@@ -1688,7 +1638,7 @@ void TimelineEventEndlessRecorder::PrintJSON(JSONStream* js,
   topLevel.AddProperty("type", "Timeline");
   {
     JSONArray events(&topLevel, "traceEvents");
-    PrintJSONMeta(events);
+    PrintJSONMeta(&events);
     PrintJSONEvents(&events, filter);
   }
   topLevel.AddPropertyTimeMicros("timeOriginMicros", TimeOriginMicros());
@@ -1699,7 +1649,7 @@ void TimelineEventEndlessRecorder::PrintTraceEvent(
     JSONStream* js,
     TimelineEventFilter* filter) {
   JSONArray events(js);
-  PrintJSONMeta(events);
+  PrintJSONMeta(&events);
   PrintJSONEvents(&events, filter);
 }
 #endif

runtime/vm/timeline.h

@@ -8,7 +8,6 @@
 #include "include/dart_tools_api.h"
 
 #include "platform/atomic.h"
-#include "platform/hashmap.h"
 #include "vm/allocation.h"
 #include "vm/bitfield.h"
 #include "vm/globals.h"
@@ -587,31 +586,6 @@ class TimelineEvent {
   DISALLOW_COPY_AND_ASSIGN(TimelineEvent);
 };
 
-class TimelineTrackMetadata {
- public:
-  TimelineTrackMetadata(intptr_t pid,
-                        intptr_t tid,
-                        Utils::CStringUniquePtr&& track_name);
-  intptr_t pid() const { return pid_; }
-  intptr_t tid() const { return tid_; }
-  const char* track_name() const { return track_name_.get(); }
-#if !defined(PRODUCT)
-  /*
-   * Prints a Chrome-format event representing the metadata stored by this
-   * object into |jsarr_events|.
-   */
-  void PrintJSON(const JSONArray& jsarr_events) const;
-#endif  // !defined(PRODUCT)
-
- private:
-  // The ID of the process that this track is associated with.
-  intptr_t pid_;
-  // The trace ID of the thread that this track is associated with.
-  intptr_t tid_;
-  // The name of this track.
-  Utils::CStringUniquePtr track_name_;
-};
-
 #ifdef SUPPORT_TIMELINE
 #define TIMELINE_DURATION(thread, stream, name)                                \
   TimelineBeginEndScope tbes(thread, Timeline::Get##stream##Stream(), name);
@@ -834,20 +808,20 @@ class IsolateTimelineEventFilter : public TimelineEventFilter {
 class TimelineEventRecorder : public MallocAllocated {
  public:
   TimelineEventRecorder();
-  virtual ~TimelineEventRecorder();
+  virtual ~TimelineEventRecorder() {}
+
+  TimelineEventBlock* GetNewBlock();
 
   // Interface method(s) which must be implemented.
 #ifndef PRODUCT
   virtual void PrintJSON(JSONStream* js, TimelineEventFilter* filter) = 0;
   virtual void PrintTraceEvent(JSONStream* js, TimelineEventFilter* filter) = 0;
 #endif
   virtual const char* name() const = 0;
-  virtual intptr_t Size() = 0;
-  TimelineEventBlock* GetNewBlock();
+
   void FinishBlock(TimelineEventBlock* block);
-  // This function must be called at least once for each thread that corresponds
-  // to a track in the trace.
-  void AddTrackMetadataBasedOnThread(const OSThread& thread);
+
+  virtual intptr_t Size() = 0;
 
  protected:
 #ifndef PRODUCT
@@ -863,7 +837,7 @@ class TimelineEventRecorder : public MallocAllocated {
 
   // Utility method(s).
 #ifndef PRODUCT
-  void PrintJSONMeta(const JSONArray& jsarr_events) const;
+  void PrintJSONMeta(JSONArray* array) const;
 #endif
   TimelineEvent* ThreadBlockStartEvent();
   void ThreadBlockCompleteEvent(TimelineEvent* event);
@@ -883,8 +857,6 @@ class TimelineEventRecorder : public MallocAllocated {
   friend class Timeline;
 
  private:
-  static const intptr_t kTrackUuidToTrackMetadataInitialCapacity = 1 << 4;
-  SimpleHashMap track_uuid_to_track_metadata_;
   DISALLOW_COPY_AND_ASSIGN(TimelineEventRecorder);
 };