diff --git a/runtime/vm/timeline.cc b/runtime/vm/timeline.cc index 925f4977d8d..37d8253065a 100644 --- a/runtime/vm/timeline.cc +++ b/runtime/vm/timeline.cc @@ -1234,6 +1234,25 @@ void TimelineBeginEndScope::EmitEnd() { event->Complete(); } +bool TimelineEventBlock::InUseLocked() const { + ASSERT(Timeline::recorder()->lock_.IsOwnedByCurrentThread()); + return in_use_; +} + +bool TimelineEventBlock::ContainsEventsThatCanBeSerializedLocked() const { + ASSERT(Timeline::recorder()->lock_.IsOwnedByCurrentThread()); + // Check that the block is not in use and not empty. |!block->in_use()| must + // be checked first because we are only holding |lock_|. Holding |lock_| + // makes it safe to call |in_use()| on any block, but only makes it safe to + // call |IsEmpty()| on blocks that are not in use. + return !InUseLocked() && !IsEmpty(); +} + +ThreadId TimelineEventBlock::ThreadIdLocked() const { + ASSERT(Timeline::recorder()->lock_.IsOwnedByCurrentThread()); + return thread_id_; +} + TimelineEventFilter::TimelineEventFilter(int64_t time_origin_micros, int64_t time_extent_micros) : time_origin_micros_(time_origin_micros), @@ -1591,7 +1610,7 @@ void TimelineEventFixedBufferRecorder::PrintEventsCommon( Timeline::ReclaimCachedBlocksFromThreads(); MutexLocker ml(&lock_); ResetTimeTracking(); - intptr_t block_offset = FindOldestBlockIndex(); + intptr_t block_offset = FindOldestBlockIndexLocked(); if (block_offset == -1) { // All blocks are in use or empty. return; @@ -1599,7 +1618,7 @@ void TimelineEventFixedBufferRecorder::PrintEventsCommon( for (intptr_t block_idx = 0; block_idx < num_blocks_; block_idx++) { TimelineEventBlock* block = &blocks_[(block_idx + block_offset) % num_blocks_]; - if (!filter.IncludeBlock(block)) { + if (!block->ContainsEventsThatCanBeSerializedLocked()) { continue; } for (intptr_t event_idx = 0; event_idx < block->length(); event_idx++) { @@ -1693,16 +1712,14 @@ void TimelineEventFixedBufferRecorder::Clear() { } } -intptr_t TimelineEventFixedBufferRecorder::FindOldestBlockIndex() const { +intptr_t TimelineEventFixedBufferRecorder::FindOldestBlockIndexLocked() const { + ASSERT(lock_.IsOwnedByCurrentThread()); int64_t earliest_time = kMaxInt64; intptr_t earliest_index = -1; for (intptr_t block_idx = 0; block_idx < num_blocks_; block_idx++) { TimelineEventBlock* block = &blocks_[block_idx]; - if (block->in_use() || block->IsEmpty()) { - // Skip in use and empty blocks. |block->in_use()| must be checked first - // because we are only holding |lock_|. Holding |lock_| makes it safe to - // call |in_use()| on any block, but only makes it safe to call - // |IsEmpty()| on blocks that are not in use. + if (!block->ContainsEventsThatCanBeSerializedLocked()) { + // Skip in use and empty blocks. continue; } if (block->LowerTimeBound() < earliest_time) { @@ -2120,7 +2137,7 @@ void TimelineEventEndlessRecorder::PrintEventsCommon( ResetTimeTracking(); for (TimelineEventBlock* current = head_; current != nullptr; current = current->next()) { - if (!filter.IncludeBlock(current)) { + if (!current->ContainsEventsThatCanBeSerializedLocked()) { continue; } intptr_t length = current->length(); @@ -2244,7 +2261,7 @@ TimelineEventBlock::~TimelineEventBlock() { #ifndef PRODUCT void TimelineEventBlock::PrintJSON(JSONStream* js) const { - ASSERT(!in_use()); + ASSERT(!InUseLocked()); JSONArray events(js); for (intptr_t i = 0; i < length(); i++) { const TimelineEvent* event = At(i); @@ -2274,29 +2291,6 @@ int64_t TimelineEventBlock::LowerTimeBound() const { return events_[0].TimeOrigin(); } -bool TimelineEventBlock::CheckBlock() { - if (length() == 0) { - return true; - } - - for (intptr_t i = 0; i < length(); i++) { - if (At(i)->thread() != thread_id()) { - return false; - } - } - - // - events have monotonically increasing timestamps. - int64_t last_time = LowerTimeBound(); - for (intptr_t i = 0; i < length(); i++) { - if (last_time > At(i)->TimeOrigin()) { - return false; - } - last_time = At(i)->TimeOrigin(); - } - - return true; -} - void TimelineEventBlock::Reset() { for (intptr_t i = 0; i < kBlockSize; i++) { // Clear any extra data. diff --git a/runtime/vm/timeline.h b/runtime/vm/timeline.h index 650cfce1939..6fe7ed31ecb 100644 --- a/runtime/vm/timeline.h +++ b/runtime/vm/timeline.h @@ -780,19 +780,17 @@ class TimelineEventBlock : public MallocAllocated { // Attempt to sniff the timestamp from the first event. int64_t LowerTimeBound() const; - // Returns false if |this| violates any of the following invariants: - // - events in the block come from one thread. - // - events have monotonically increasing timestamps. - bool CheckBlock(); - // Call Reset on all events and set length to 0. void Reset(); // Only safe to access under the recorder's lock. - bool in_use() const { return in_use_; } + inline bool InUseLocked() const; // Only safe to access under the recorder's lock. - ThreadId thread_id() const { return thread_id_; } + inline bool ContainsEventsThatCanBeSerializedLocked() const; + + // Only safe to access under the recorder's lock. + inline ThreadId ThreadIdLocked() const; protected: #ifndef PRODUCT @@ -833,17 +831,6 @@ class TimelineEventFilter : public ValueObject { virtual ~TimelineEventFilter(); - bool IncludeBlock(TimelineEventBlock* block) const { - if (block == nullptr) { - return false; - } - // Check that the block is not in use and not empty. |!block->in_use()| must - // be checked first because we are only holding |lock_|. Holding |lock_| - // makes it safe to call |in_use()| on any block, but only makes it safe to - // call |IsEmpty()| on blocks that are not in use. - return !block->in_use() && !block->IsEmpty(); - } - virtual bool IncludeEvent(TimelineEvent* event) const { if (event == nullptr) { return false; @@ -951,6 +938,7 @@ class TimelineEventRecorder : public MallocAllocated { int64_t time_high_micros_; friend class TimelineEvent; + friend class TimelineEventBlock; friend class TimelineStream; friend class TimelineTestHelper; friend class Timeline; @@ -994,7 +982,8 @@ class TimelineEventFixedBufferRecorder : public TimelineEventRecorder { TimelineEvent* StartEvent(); void CompleteEvent(TimelineEvent* event); TimelineEventBlock* GetHeadBlockLocked(); - intptr_t FindOldestBlockIndex() const; + // Only safe to call when holding |lock_|. + intptr_t FindOldestBlockIndexLocked() const; void Clear(); #ifndef PRODUCT diff --git a/runtime/vm/timeline_test.cc b/runtime/vm/timeline_test.cc index f841c63dbd5..dc13f1ca8e0 100644 --- a/runtime/vm/timeline_test.cc +++ b/runtime/vm/timeline_test.cc @@ -383,15 +383,15 @@ TEST_CASE(TimelineRingRecorderJSONOrder) { TEST_CASE(TimelineTrackMetadataRace) { struct ReportMetadataArguments { Monitor& synchronization_monitor; - TimelineEventRingRecorder& recorder; + TimelineEventRecorder& recorder; ThreadJoinId join_id = OSThread::kInvalidThreadJoinId; }; Monitor synchronization_monitor; - TimelineEventRingRecorder recorder; + TimelineEventRecorder& recorder = *Timeline::recorder(); - // 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 + // Try concurrently reading from / writing to the metadata map. It is not + // 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;