dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[VM/Timeline] Add assertions to check that the recorder's lock is held where required

Browse Source 
TEST=Checked that the timeline in DevTools still looked correct, and
checked that no assertions failed when fetching the timeline

Change-Id: Ia7ed4b8faf4e7c9e8871ecca3111a82e7f298318
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/300420
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Siva Annamalai <asiva@google.com>
This commit is contained in:
Derek Xu
2023-05-03 19:17:11 +00:00
committed by Commit Queue
parent e82a5a2ff3
commit da971bbf13
3 changed files with 39 additions and 56 deletions
Download Patch File Download Diff File Expand all files Collapse all files
runtime/vm/timeline.cc
+27 -33
View File
@@ -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.
runtime/vm/timeline.h
+8 -19
View File
@@ -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
runtime/vm/timeline_test.cc
+4 -4
View File
@@ -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;