diff --git a/runtime/vm/os_thread.cc b/runtime/vm/os_thread.cc index e04dc4a13a4..e101536ee41 100644 --- a/runtime/vm/os_thread.cc +++ b/runtime/vm/os_thread.cc @@ -27,9 +27,7 @@ inline void UpdateTimelineTrackMetadata(const OSThread& thread) { RecorderLockScope rl; TimelineEventRecorder* recorder = Timeline::recorder(); if (recorder != nullptr && !rl.IsShuttingDown()) { - recorder->AddTrackMetadataBasedOnThread( - OS::ProcessId(), OSThread::ThreadIdToIntPtr(thread.trace_id()), - thread.name()); + recorder->AddTrackMetadataBasedOnThread(thread); } } #endif // defined(SUPPORT_TIMELINE) diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc index 43a17b0097e..f70b6b444d1 100644 --- a/runtime/vm/timeline.cc +++ b/runtime/vm/timeline.cc @@ -1072,10 +1072,8 @@ TimelineEventRecorder::TimelineEventRecorder() // were initialized before this point. OSThreadIterator it; while (it.HasNext()) { - OSThread& thread = *it.Next(); - AddTrackMetadataBasedOnThread(OS::ProcessId(), - OSThread::ThreadIdToIntPtr(thread.trace_id()), - thread.name()); + OSThread* thread = it.Next(); + AddTrackMetadataBasedOnThread(*thread); } } @@ -1248,9 +1246,7 @@ TimelineEventBlock* TimelineEventRecorder::GetNewBlock() { } void TimelineEventRecorder::AddTrackMetadataBasedOnThread( - const intptr_t process_id, - const intptr_t trace_id, - const char* thread_name) { + const OSThread& thread) { 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 update the map in these cases. @@ -1258,19 +1254,23 @@ void TimelineEventRecorder::AddTrackMetadataBasedOnThread( strcmp("systrace", FLAG_timeline_recorder) != 0) { MutexLocker ml(&track_uuid_to_track_metadata_lock_); - void* key = reinterpret_cast(trace_id); - const intptr_t hash = Utils::WordHash(trace_id); + intptr_t pid = OS::ProcessId(); + intptr_t tid = OSThread::ThreadIdToIntPtr(thread.trace_id()); + const char* thread_name = thread.name(); + + void* key = reinterpret_cast(tid); + const intptr_t hash = Utils::WordHash(tid); SimpleHashMap::Entry* entry = track_uuid_to_track_metadata_.Lookup(key, hash, true); if (entry->value == nullptr) { entry->value = new TimelineTrackMetadata( - process_id, trace_id, + pid, tid, Utils::CreateCStringUniquePtr( Utils::StrDup(thread_name == nullptr ? "" : thread_name))); } else { TimelineTrackMetadata* value = static_cast(entry->value); - ASSERT(process_id == value->pid()); + ASSERT(pid == value->pid()); value->set_track_name(Utils::CreateCStringUniquePtr( Utils::StrDup(thread_name == nullptr ? "" : thread_name))); } diff --git a/runtime/vm/timeline.h b/runtime/vm/timeline.h index edbc12575e8..962f935c999 100644 --- a/runtime/vm/timeline.h +++ b/runtime/vm/timeline.h @@ -848,9 +848,7 @@ class TimelineEventRecorder : public MallocAllocated { 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 intptr_t process_id, - const intptr_t trace_id, - const char* thread_name); + void AddTrackMetadataBasedOnThread(const OSThread& thread); protected: #ifndef PRODUCT diff --git a/runtime/vm/timeline_test.cc b/runtime/vm/timeline_test.cc index 61206121c48..ee3ff18f1e7 100644 --- a/runtime/vm/timeline_test.cc +++ b/runtime/vm/timeline_test.cc @@ -3,7 +3,6 @@ // BSD-style license that can be found in the LICENSE file. #include -#include #include "platform/assert.h" @@ -374,56 +373,6 @@ TEST_CASE(TimelineRingRecorderJSONOrder) { EXPECT(alpha < beta); } -TEST_CASE(TimelineTrackMetadataRace) { - struct ReportMetadataArguments { - TimelineEventRingRecorder* recorder; - ThreadJoinId join_id = OSThread::kInvalidThreadJoinId; - }; - - TimelineEventRingRecorder recorder; - const intptr_t fake_process_id = 1; - const intptr_t fake_trace_id = 1; - - // Try concurrently reading from / writing to the metadata map. I don't think - // it's possible to assert anything about the outcome, because of scheduling - // uncertainty. This test is just used to ensure that TSAN checks the metadata - // map code. - JSONStream js; - TimelineEventFilter filter; - const ReportMetadataArguments report_metadata_1_arguments{&recorder}; - const ReportMetadataArguments report_metadata_2_arguments{&recorder}; - OSThread::Start( - "ReportMetadata1", - [](uword arguments_ptr) { - ReportMetadataArguments& arguments = - *reinterpret_cast(arguments_ptr); - arguments.recorder->AddTrackMetadataBasedOnThread( - fake_process_id, fake_trace_id, "Thread 1"); - arguments.join_id = - OSThread::GetCurrentThreadJoinId(OSThread::Current()); - }, - reinterpret_cast(&report_metadata_1_arguments)); - OSThread::Start( - "ReportMetadata2", - [](uword arguments_ptr) { - ReportMetadataArguments& arguments = - *reinterpret_cast(arguments_ptr); - arguments.recorder->AddTrackMetadataBasedOnThread( - fake_process_id, fake_trace_id, "Incorrect Name"); - arguments.join_id = - OSThread::GetCurrentThreadJoinId(OSThread::Current()); - }, - reinterpret_cast(&report_metadata_2_arguments)); - recorder.PrintJSON(&js, &filter); - while ( - report_metadata_1_arguments.join_id == OSThread::kInvalidThreadJoinId || - report_metadata_2_arguments.join_id == OSThread::kInvalidThreadJoinId) { - // Spin until the join IDs have been set. - } - OSThread::Join(report_metadata_1_arguments.join_id); - OSThread::Join(report_metadata_2_arguments.join_id); -} - #endif // !PRODUCT #if defined(SUPPORT_TIMELINE)