Merge "ART: Fix GC pause reporting"

5 files changed, 45 insertions, 30 deletions

diff --git a/runtime/gc/collector/concurrent_copying.cc b/runtime/gc/collector/concurrent_copying.cc
index 6044053b4f..c9e5746990 100644
--- a/runtime/gc/collector/concurrent_copying.cc
+++ b/runtime/gc/collector/concurrent_copying.cc
@@ -25,6 +25,7 @@
 #include "gc/accounting/heap_bitmap-inl.h"
 #include "gc/accounting/mod_union_table-inl.h"
 #include "gc/accounting/space_bitmap-inl.h"
+#include "gc/gc_pause_listener.h"
 #include "gc/reference_processor.h"
 #include "gc/space/image_space.h"
 #include "gc/space/space-inl.h"
@@ -139,7 +140,7 @@ void ConcurrentCopying::RunPhases() {
   // Verify no from space refs. This causes a pause.
   if (kEnableNoFromSpaceRefsVerification || kIsDebugBuild) {
     TimingLogger::ScopedTiming split("(Paused)VerifyNoFromSpaceReferences", GetTimings());
-    ScopedPause pause(this);
+    ScopedPause pause(this, false);
     CheckEmptyMarkStack();
     if (kVerboseMode) {
       LOG(INFO) << "Verifying no from-space refs";
@@ -439,8 +440,27 @@ void ConcurrentCopying::FlipThreadRoots() {
   gc_barrier_->Init(self, 0);
   ThreadFlipVisitor thread_flip_visitor(this, heap_->use_tlab_);
   FlipCallback flip_callback(this);
+
+  // This is the point where Concurrent-Copying will pause all threads. We report a pause here, if
+  // necessary. This is slightly over-reporting, as this includes the time to actually suspend
+  // threads.
+  {
+    GcPauseListener* pause_listener = GetHeap()->GetGcPauseListener();
+    if (pause_listener != nullptr) {
+      pause_listener->StartPause();
+    }
+  }
+
   size_t barrier_count = Runtime::Current()->FlipThreadRoots(
       &thread_flip_visitor, &flip_callback, this);
+
+  {
+    GcPauseListener* pause_listener = GetHeap()->GetGcPauseListener();
+    if (pause_listener != nullptr) {
+      pause_listener->EndPause();
+    }
+  }
+
   {
     ScopedThreadStateChange tsc(self, kWaitingForCheckPointsToRun);
     gc_barrier_->Increment(self, barrier_count);
diff --git a/runtime/gc/collector/garbage_collector.cc b/runtime/gc/collector/garbage_collector.cc
index 01bcb7df19..14fd332b57 100644
--- a/runtime/gc/collector/garbage_collector.cc
+++ b/runtime/gc/collector/garbage_collector.cc
@@ -158,22 +158,26 @@ void GarbageCollector::ResetMeasurements() {
   total_freed_bytes_ = 0;
 }
 
-GarbageCollector::ScopedPause::ScopedPause(GarbageCollector* collector)
-    : start_time_(NanoTime()), collector_(collector) {
+GarbageCollector::ScopedPause::ScopedPause(GarbageCollector* collector, bool with_reporting)
+    : start_time_(NanoTime()), collector_(collector), with_reporting_(with_reporting) {
   Runtime* runtime = Runtime::Current();
   runtime->GetThreadList()->SuspendAll(__FUNCTION__);
-  GcPauseListener* pause_listener = runtime->GetHeap()->GetGcPauseListener();
-  if (pause_listener != nullptr) {
-    pause_listener->StartPause();
+  if (with_reporting) {
+    GcPauseListener* pause_listener = runtime->GetHeap()->GetGcPauseListener();
+    if (pause_listener != nullptr) {
+      pause_listener->StartPause();
+    }
   }
 }
 
 GarbageCollector::ScopedPause::~ScopedPause() {
   collector_->RegisterPause(NanoTime() - start_time_);
   Runtime* runtime = Runtime::Current();
-  GcPauseListener* pause_listener = runtime->GetHeap()->GetGcPauseListener();
-  if (pause_listener != nullptr) {
-    pause_listener->EndPause();
+  if (with_reporting_) {
+    GcPauseListener* pause_listener = runtime->GetHeap()->GetGcPauseListener();
+    if (pause_listener != nullptr) {
+      pause_listener->EndPause();
+    }
   }
   runtime->GetThreadList()->ResumeAll();
 }
diff --git a/runtime/gc/collector/garbage_collector.h b/runtime/gc/collector/garbage_collector.h
index 0177e2a1ad..95601d736d 100644
--- a/runtime/gc/collector/garbage_collector.h
+++ b/runtime/gc/collector/garbage_collector.h
@@ -126,12 +126,14 @@ class GarbageCollector : public RootVisitor, public IsMarkedVisitor, public Mark
  public:
   class SCOPED_LOCKABLE ScopedPause {
    public:
-    explicit ScopedPause(GarbageCollector* collector) EXCLUSIVE_LOCK_FUNCTION(Locks::mutator_lock_);
+    explicit ScopedPause(GarbageCollector* collector, bool with_reporting = true)
+        EXCLUSIVE_LOCK_FUNCTION(Locks::mutator_lock_);
     ~ScopedPause() UNLOCK_FUNCTION();
 
    private:
     const uint64_t start_time_;
     GarbageCollector* const collector_;
+    bool with_reporting_;
   };
 
   GarbageCollector(Heap* heap, const std::string& name);
diff --git a/runtime/gc/heap.cc b/runtime/gc/heap.cc
index 70449797c1..f5bf935323 100644
--- a/runtime/gc/heap.cc
+++ b/runtime/gc/heap.cc
@@ -3352,7 +3352,7 @@ void Heap::PreGcVerificationPaused(collector::GarbageCollector* gc) {
 
 void Heap::PreGcVerification(collector::GarbageCollector* gc) {
   if (verify_pre_gc_heap_ || verify_missing_card_marks_ || verify_mod_union_table_) {
-    collector::GarbageCollector::ScopedPause pause(gc);
+    collector::GarbageCollector::ScopedPause pause(gc, false);
     PreGcVerificationPaused(gc);
   }
 }
@@ -3420,7 +3420,7 @@ void Heap::PostGcVerificationPaused(collector::GarbageCollector* gc) {
 
 void Heap::PostGcVerification(collector::GarbageCollector* gc) {
   if (verify_system_weaks_ || verify_post_gc_rosalloc_ || verify_post_gc_heap_) {
-    collector::GarbageCollector::ScopedPause pause(gc);
+    collector::GarbageCollector::ScopedPause pause(gc, false);
     PostGcVerificationPaused(gc);
   }
 }
diff --git a/test/908-gc-start-finish/gc_callbacks.cc b/test/908-gc-start-finish/gc_callbacks.cc
index 59801ff648..8f96ee63ef 100644
--- a/test/908-gc-start-finish/gc_callbacks.cc
+++ b/test/908-gc-start-finish/gc_callbacks.cc
@@ -38,43 +38,32 @@ static void JNICALL GarbageCollectionStart(jvmtiEnv* ti_env ATTRIBUTE_UNUSED) {
 }
 
 extern "C" JNIEXPORT void JNICALL Java_Main_setupGcCallback(
-    JNIEnv* env ATTRIBUTE_UNUSED, jclass klass ATTRIBUTE_UNUSED) {
+    JNIEnv* env, jclass klass ATTRIBUTE_UNUSED) {
   jvmtiEventCallbacks callbacks;
   memset(&callbacks, 0, sizeof(jvmtiEventCallbacks));
   callbacks.GarbageCollectionFinish = GarbageCollectionFinish;
   callbacks.GarbageCollectionStart = GarbageCollectionStart;
 
   jvmtiError ret = jvmti_env->SetEventCallbacks(&callbacks, sizeof(callbacks));
-  if (ret != JVMTI_ERROR_NONE) {
-    char* err;
-    jvmti_env->GetErrorName(ret, &err);
-    printf("Error setting callbacks: %s\n", err);
-    jvmti_env->Deallocate(reinterpret_cast<unsigned char*>(err));
-  }
+  JvmtiErrorToException(env, ret);
 }
 
-extern "C" JNIEXPORT void JNICALL Java_Main_enableGcTracking(JNIEnv* env ATTRIBUTE_UNUSED,
+extern "C" JNIEXPORT void JNICALL Java_Main_enableGcTracking(JNIEnv* env,
                                                              jclass klass ATTRIBUTE_UNUSED,
                                                              jboolean enable) {
   jvmtiError ret = jvmti_env->SetEventNotificationMode(
       enable ? JVMTI_ENABLE : JVMTI_DISABLE,
       JVMTI_EVENT_GARBAGE_COLLECTION_START,
       nullptr);
-  if (ret != JVMTI_ERROR_NONE) {
-    char* err;
-    jvmti_env->GetErrorName(ret, &err);
-    printf("Error enabling/disabling gc callbacks: %s\n", err);
-    jvmti_env->Deallocate(reinterpret_cast<unsigned char*>(err));
+  if (JvmtiErrorToException(env, ret)) {
+    return;
   }
   ret = jvmti_env->SetEventNotificationMode(
       enable ? JVMTI_ENABLE : JVMTI_DISABLE,
       JVMTI_EVENT_GARBAGE_COLLECTION_FINISH,
       nullptr);
-  if (ret != JVMTI_ERROR_NONE) {
-    char* err;
-    jvmti_env->GetErrorName(ret, &err);
-    printf("Error enabling/disabling gc callbacks: %s\n", err);
-    jvmti_env->Deallocate(reinterpret_cast<unsigned char*>(err));
+  if (JvmtiErrorToException(env, ret)) {
+    return;
   }
 }