ZGC: Review 8: Rename and simplify iteration functions

Author: Stefan Karlsson

src/hotspot/share/gc/z/zVerify.cpp

@@ -114,10 +114,11 @@ class ZVerifyStack : public OopClosure {
     ZStackWatermark* stack_watermark = StackWatermarkSet::get<ZStackWatermark>(jt, StackWatermarkSet::gc);
 
     if (_cl->verify_fixed()) {
-      assert(!stack_watermark->should_start_iteration(), "Should already have been fixed");
+      assert(stack_watermark->iteration_started(), "Should already have been fixed");
+      assert(stack_watermark->iteration_completed(), "Should already have been fixed");
     } else {
       // We don't really know the state of the stack, verify watermark.
-      if (stack_watermark->should_start_iteration()) {
+      if (!stack_watermark->iteration_started()) {
         _verifying_bad_frames = true;
       } else {
         // Not time yet to verify bad frames

src/hotspot/share/runtime/stackWatermark.cpp

@@ -175,25 +175,13 @@ StackWatermark::~StackWatermark() {
 bool StackWatermark::is_frame_safe(frame f) {
   MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
   uint32_t state = Atomic::load(&_state);
-  if (StackWatermarkState::epoch(state) != epoch_id()) {
+  if (!iteration_started(state)) {
     return false;
   }
-  if (StackWatermarkState::is_done(state)) {
+  if (iteration_completed(state)) {
     return true;
   }
-  if (_iterator != NULL) {
-    return reinterpret_cast<uintptr_t>(f.sp()) < _iterator->caller();
-  }
-  return true;
-}
-
-bool StackWatermark::should_start_iteration() const {
-  return StackWatermarkState::epoch(_state) != epoch_id();
-}
-
-bool StackWatermark::should_start_iteration_acquire() const {
-  uint32_t state = Atomic::load_acquire(&_state);
-  return StackWatermarkState::epoch(state) != epoch_id();
+  return reinterpret_cast<uintptr_t>(f.sp()) < _iterator->caller();
 }
 
 void StackWatermark::start_iteration_impl(void* context) {
@@ -241,9 +229,9 @@ void StackWatermark::update_watermark() {
 
 void StackWatermark::process_one() {
   MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
-  if (should_start_iteration()) {
+  if (!iteration_started()) {
     start_iteration_impl(NULL /* context */);
-  } else if (_iterator != NULL) {
+  } else if (!iteration_completed()) {
     _iterator->process_one(NULL /* context */);
     update_watermark();
   }
@@ -255,36 +243,48 @@ uintptr_t StackWatermark::watermark() {
 
 uintptr_t StackWatermark::last_processed() {
   MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
-  if (should_start_iteration()) {
+  if (!iteration_started()) {
     // Stale state; no last processed
     return 0;
   }
-  if (watermark() == 0) {
+  if (iteration_completed()) {
     // Already processed all; no last processed
     return 0;
   }
-  if (_iterator == NULL) {
-    // No frames to processed; no last processed
-    return 0;
-  }
   return _iterator->caller();
 }
 
+bool StackWatermark::iteration_started() const {
+  return iteration_started(Atomic::load(&_state));
+}
+
+bool StackWatermark::iteration_started_acquire() const {
+  return iteration_started(Atomic::load_acquire(&_state));
+}
+
+bool StackWatermark::iteration_completed() const {
+  return iteration_completed(Atomic::load(&_state));
+}
+
+bool StackWatermark::iteration_completed_acquire() const {
+  return iteration_completed(Atomic::load_acquire(&_state));
+}
+
 void StackWatermark::start_iteration() {
-  if (should_start_iteration_acquire()) {
+  if (!iteration_started_acquire()) {
     MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
-    if (should_start_iteration()) {
+    if (!iteration_started()) {
       start_iteration_impl(NULL /* context */);
     }
   }
 }
 
 void StackWatermark::finish_iteration(void* context) {
   MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
-  if (should_start_iteration()) {
+  if (!iteration_started()) {
     start_iteration_impl(context);
   }
-  if (_iterator != NULL) {
+  if (!iteration_completed()) {
     _iterator->process_all(context);
     update_watermark();
   }

src/hotspot/share/runtime/stackWatermark.hpp

@@ -78,24 +78,30 @@ class StackWatermark : public CHeapObj<mtInternal> {
   // opposed to due to frames being unwinded by the owning thread.
   virtual bool process_on_iteration() { return true; }
 
+  bool iteration_started(uint32_t state) const;
+  bool iteration_completed(uint32_t state) const;
+
 public:
   StackWatermark(JavaThread* jt, StackWatermarkSet::Kind kind, uint32_t epoch);
   virtual ~StackWatermark();
 
-  uintptr_t watermark();
 
   // StackWatermarkSet support
   StackWatermarkSet::Kind kind() const { return _kind; }
   StackWatermark* next() const { return _next; }
   void set_next(StackWatermark* n) { _next = n; }
 
-  bool should_start_iteration() const;
-  bool should_start_iteration_acquire() const;
-
+  uintptr_t watermark();
   uintptr_t last_processed();
 
+  bool iteration_started() const;
+  bool iteration_started_acquire() const;
+  bool iteration_completed() const;
+  bool iteration_completed_acquire() const;
+
   void before_unwind();
   void after_unwind();
+
   void on_iteration(frame fr);
   void start_iteration();
   void finish_iteration(void* context);

src/hotspot/share/runtime/stackWatermark.inline.hpp

@@ -51,11 +51,19 @@ inline bool StackWatermark::has_barrier(frame& f) {
   return false;
 }
 
+inline bool StackWatermark::iteration_started(uint32_t state) const {
+  return StackWatermarkState::epoch(state) == epoch_id();
+}
+
+inline bool StackWatermark::iteration_completed(uint32_t state) const {
+  assert(iteration_started(state), "Check is only valid if iteration has been started");
+  return StackWatermarkState::is_done(state);
+}
+
 inline void StackWatermark::ensure_safe(frame f) {
-  assert(!should_start_iteration(), "Iteration should already have started");
+  assert(iteration_started(), "Iteration should already have started");
 
-  uint32_t state = Atomic::load_acquire(&_state);
-  if (StackWatermarkState::is_done(state)) {
+  if (iteration_completed_acquire()) {
     return;
   }