Change Checkpoint API to return total number of checkpoints
Fixes a race condition with SetStateUnsafe that caused some
warnings in the Barrier::~Barrier.
The race was:
RunCheckpoint sees suspended thread, runs the checkpoint. Inside the
checkpoint, the thread state had changed to runnable by
SetStateUnsafe. This occasionally caused more Barrier::Pass than
expected.
The fix is to return the total number of checkpoints instead of just
the runnable ones.
Bug: 24191051
Change-Id: If15a933ed4c8efa66a5f27cd5feaa2e5957ae804
diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index e433b8d..d7e8f81 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -339,9 +339,7 @@
<< thread->GetState() << " thread " << thread << " self " << self;
// If thread is a running mutator, then act on behalf of the garbage collector.
// See the code in ThreadList::RunCheckpoint.
- if (thread->GetState() == kRunnable) {
- concurrent_copying_->GetBarrier().Pass(self);
- }
+ concurrent_copying_->GetBarrier().Pass(self);
}
private:
@@ -514,9 +512,7 @@
thread->SetIsGcMarking(false);
// If thread is a running mutator, then act on behalf of the garbage collector.
// See the code in ThreadList::RunCheckpoint.
- if (thread->GetState() == kRunnable) {
- concurrent_copying_->GetBarrier().Pass(self);
- }
+ concurrent_copying_->GetBarrier().Pass(self);
}
private:
@@ -937,9 +933,7 @@
}
// If thread is a running mutator, then act on behalf of the garbage collector.
// See the code in ThreadList::RunCheckpoint.
- if (thread->GetState() == kRunnable) {
- concurrent_copying_->GetBarrier().Pass(self);
- }
+ concurrent_copying_->GetBarrier().Pass(self);
}
private:
diff --git a/runtime/gc/collector/mark_sweep.cc b/runtime/gc/collector/mark_sweep.cc
index 77a288b..db516a0 100644
--- a/runtime/gc/collector/mark_sweep.cc
+++ b/runtime/gc/collector/mark_sweep.cc
@@ -1146,9 +1146,7 @@
}
// If thread is a running mutator, then act on behalf of the garbage collector.
// See the code in ThreadList::RunCheckpoint.
- if (thread->GetState() == kRunnable) {
- mark_sweep_->GetBarrier().Pass(self);
- }
+ mark_sweep_->GetBarrier().Pass(self);
}
private:
diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc
index 1d38525..ab93142 100644
--- a/runtime/gc/heap.cc
+++ b/runtime/gc/heap.cc
@@ -1291,9 +1291,7 @@
ATRACE_END();
// If thread is a running mutator, then act on behalf of the trim thread.
// See the code in ThreadList::RunCheckpoint.
- if (thread->GetState() == kRunnable) {
- barrier_->Pass(Thread::Current());
- }
+ barrier_->Pass(Thread::Current());
}
private:
diff --git a/runtime/jit/jit_code_cache.cc b/runtime/jit/jit_code_cache.cc
index 60568b2..cfccec8 100644
--- a/runtime/jit/jit_code_cache.cc
+++ b/runtime/jit/jit_code_cache.cc
@@ -369,9 +369,7 @@
DCHECK(thread == Thread::Current() || thread->IsSuspended());
MarkCodeVisitor visitor(thread, code_cache_);
visitor.WalkStack();
- if (thread->GetState() == kRunnable) {
- barrier_->Pass(Thread::Current());
- }
+ barrier_->Pass(Thread::Current());
}
private:
diff --git a/runtime/thread_list.cc b/runtime/thread_list.cc
index bdd5d10..ed859cf 100644
--- a/runtime/thread_list.cc
+++ b/runtime/thread_list.cc
@@ -195,9 +195,7 @@
MutexLock mu(self, *Locks::logging_lock_);
*os_ << local_os.str();
}
- if (thread->GetState() == kRunnable) {
- barrier_.Pass(self);
- }
+ barrier_.Pass(self);
}
void WaitForThreadsToRunThroughCheckpoint(size_t threads_running_checkpoint) {
@@ -285,12 +283,12 @@
// manually called.
MutexLock mu(self, *Locks::thread_list_lock_);
MutexLock mu2(self, *Locks::thread_suspend_count_lock_);
+ count = list_.size();
for (const auto& thread : list_) {
if (thread != self) {
while (true) {
if (thread->RequestCheckpoint(checkpoint_function)) {
// This thread will run its checkpoint some time in the near future.
- count++;
break;
} else {
// We are probably suspended, try to make sure that we stay suspended.
diff --git a/runtime/thread_list.h b/runtime/thread_list.h
index c727432..f0933f7 100644
--- a/runtime/thread_list.h
+++ b/runtime/thread_list.h
@@ -91,7 +91,8 @@
Thread* FindThreadByThreadId(uint32_t thin_lock_id);
// Run a checkpoint on threads, running threads are not suspended but run the checkpoint inside
- // of the suspend check. Returns how many checkpoints we should expect to run.
+ // of the suspend check. Returns how many checkpoints that are expected to run, including for
+ // already suspended threads.
size_t RunCheckpoint(Closure* checkpoint_function)
REQUIRES(!Locks::thread_list_lock_, !Locks::thread_suspend_count_lock_);