Revert "[VM/Timeline] Improve handling of track metadata"
This reverts commit 3787601dd0b9a7abe1d1052d2ca28f3cfb0f68da.
Reason for revert: Introduces a crash that's blocking the Dart->Flutter roller: https://github.com/flutter/flutter/issues/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 <bkonyi@google.com>
> Commit-Queue: Derek Xu <derekx@google.com>
TBR=bkonyi@google.com,derekx@google.com,dart-scoped@luci-project-accounts.iam.gserviceaccount.com
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 <derekx@google.com>
Reviewed-by: Derek Xu <derekx@google.com>
Auto-Submit: Brandon DeRosier <bdero@google.com>
diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc
index 24195c2..d808702 100644
--- a/runtime/vm/timeline.cc
+++ b/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 @@
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 @@
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 @@
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 @@
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 @@
topLevel.AddProperty("type", "Timeline");
{
JSONArray events(&topLevel, "traceEvents");
- PrintJSONMeta(events);
+ PrintJSONMeta(&events);
PrintJSONEvents(&events, filter);
}
topLevel.AddPropertyTimeMicros("timeOriginMicros", TimeOriginMicros());
@@ -1343,7 +1293,7 @@
JSONStream* js,
TimelineEventFilter* filter) {
JSONArray events(js);
- PrintJSONMeta(events);
+ PrintJSONMeta(&events);
PrintJSONEvents(&events, filter);
}
#endif
@@ -1688,7 +1638,7 @@
topLevel.AddProperty("type", "Timeline");
{
JSONArray events(&topLevel, "traceEvents");
- PrintJSONMeta(events);
+ PrintJSONMeta(&events);
PrintJSONEvents(&events, filter);
}
topLevel.AddPropertyTimeMicros("timeOriginMicros", TimeOriginMicros());
@@ -1699,7 +1649,7 @@
JSONStream* js,
TimelineEventFilter* filter) {
JSONArray events(js);
- PrintJSONMeta(events);
+ PrintJSONMeta(&events);
PrintJSONEvents(&events, filter);
}
#endif
diff --git a/runtime/vm/timeline.h b/runtime/vm/timeline.h
index 471c4f9..355fd19 100644
--- a/runtime/vm/timeline.h
+++ b/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 @@
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,7 +808,9 @@
class TimelineEventRecorder : public MallocAllocated {
public:
TimelineEventRecorder();
- virtual ~TimelineEventRecorder();
+ virtual ~TimelineEventRecorder() {}
+
+ TimelineEventBlock* GetNewBlock();
// Interface method(s) which must be implemented.
#ifndef PRODUCT
@@ -842,12 +818,10 @@
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 @@
// 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 @@
friend class Timeline;
private:
- static const intptr_t kTrackUuidToTrackMetadataInitialCapacity = 1 << 4;
- SimpleHashMap track_uuid_to_track_metadata_;
DISALLOW_COPY_AND_ASSIGN(TimelineEventRecorder);
};
