8203299: StringPoolBuffer access covered by exclusive access invariant, remove (problematic) cas operations

Reviewed-by: egahlin

src/hotspot/share/jfr/recorder/storage/jfrBuffer.hpp

@@ -25,7 +25,6 @@
 #ifndef SHARE_VM_JFR_RECORDER_STORAGE_JFRBUFFER_HPP
 #define SHARE_VM_JFR_RECORDER_STORAGE_JFRBUFFER_HPP
 
-#include "jni.h"
 #include "memory/allocation.hpp"
 
 //
@@ -34,7 +33,7 @@
 // u1* _pos <-- next store position
 // u1* _top <-- next unflushed position
 //
-// const void* _identity <<-- acquired by
+// const void* _identity <-- acquired by
 //
 // Must be the owner before attempting stores.
 // Use acquire() and/or try_acquire() for exclusive access

src/hotspot/share/jfr/recorder/stringpool/jfrStringPool.cpp

@@ -151,7 +151,7 @@ class StringPoolWriteOp  {
   StringPoolWriteOp(JfrChunkWriter& writer, Thread* thread) : _writer(writer), _thread(thread), _strings_processed(0) {}
   bool write(Type* buffer, const u1* data, size_t size) {
     buffer->acquire(_thread); // blocking
-    const u4 nof_strings_used = (const u4)buffer->string_count();
+    const uint64_t nof_strings_used = buffer->string_count();
     assert(nof_strings_used > 0, "invariant");
     buffer->set_string_top(buffer->string_top() + nof_strings_used);
     // "size processed" for string pool buffers is the number of processed string elements
@@ -208,10 +208,10 @@ class StringPoolBufferDiscarder {
       return true;
     }
     buffer->set_top(current_top + unflushed_size);
-    const u4 nof_strings_used = buffer->string_count();
+    const uint64_t nof_strings_used = buffer->string_count();
     buffer->set_string_top(buffer->string_top() + nof_strings_used);
     // "size processed" for string pool buffers is the number of string elements
-    _processed += nof_strings_used;
+    _processed += (size_t)nof_strings_used;
     buffer->release();
     return true;
   }

src/hotspot/share/jfr/recorder/stringpool/jfrStringPoolBuffer.cpp

@@ -24,13 +24,11 @@
 
 #include "precompiled.hpp"
 #include "jfr/recorder/stringpool/jfrStringPoolBuffer.hpp"
-#include "runtime/atomic.hpp"
-#include "runtime/orderAccess.hpp"
-#include "runtime/thread.inline.hpp"
 
 JfrStringPoolBuffer::JfrStringPoolBuffer() : JfrBuffer(), _string_count_pos(0), _string_count_top(0) {}
 
 void JfrStringPoolBuffer::reinitialize() {
+  assert(acquired_by_self() || retired(), "invariant");
   concurrent_top();
   set_pos((start()));
   set_string_pos(0);
@@ -39,35 +37,31 @@ void JfrStringPoolBuffer::reinitialize() {
 }
 
 uint64_t JfrStringPoolBuffer::string_pos() const {
-  return OrderAccess::load_acquire(&_string_count_pos);
+  assert(acquired_by_self() || retired(), "invariant");
+  return _string_count_pos;
 }
 
 uint64_t JfrStringPoolBuffer::string_top() const {
-  return OrderAccess::load_acquire(&_string_count_top);
+  assert(acquired_by_self() || retired(), "invariant");
+  return _string_count_top;
 }
 
 uint64_t JfrStringPoolBuffer::string_count() const {
+  assert(acquired_by_self() || retired(), "invariant");
   return string_pos() - string_top();
 }
 
 void JfrStringPoolBuffer::set_string_pos(uint64_t value) {
-  Atomic::store(value, &_string_count_pos);
+  assert(acquired_by_self() || retired(), "invariant");
+  _string_count_pos = value;
 }
 
 void JfrStringPoolBuffer::increment(uint64_t value) {
-#if !(defined(ARM) || defined(IA32))
-  Atomic::add(value, &_string_count_pos);
-#else
-  // TODO: This should be fixed in Atomic::add handling for 32-bit platforms,
-  // see JDK-8203283. We workaround the absence of support right here.
-  uint64_t cur, val;
-  do {
-     cur = Atomic::load(&_string_count_top);
-     val = cur + value;
-  } while (Atomic::cmpxchg(val, &_string_count_pos, cur) != cur);
-#endif
+  assert(acquired_by_self() || retired(), "invariant");
+  ++_string_count_pos;
 }
 
 void JfrStringPoolBuffer::set_string_top(uint64_t value) {
-  Atomic::store(value, &_string_count_top);
+  assert(acquired_by_self() || retired(), "invariant");
+  _string_count_top = value;
 }

src/hotspot/share/jfr/recorder/stringpool/jfrStringPoolBuffer.hpp

@@ -26,7 +26,6 @@
 #define SHARE_VM_JFR_RECORDER_STRINGPOOL_JFRSTRINGPOOLBUFFER_HPP
 
 #include "jfr/recorder/storage/jfrBuffer.hpp"
-#include "utilities/globalDefinitions.hpp"
 
 class JfrStringPoolBuffer : public JfrBuffer {
  private:

src/hotspot/share/jfr/utilities/jfrAllocation.cpp

@@ -53,8 +53,8 @@ static volatile jlong _live_set_bytes = 0;
 
 static void add(size_t alloc_size) {
   if (!JfrRecorder::is_created()) {
-    const jlong total_allocated = atomic_add_jlong(alloc_size, &_allocated_bytes);
-    const jlong current_live_set = atomic_add_jlong(alloc_size, &_live_set_bytes);
+    const jlong total_allocated = atomic_add_jlong((jlong)alloc_size, &_allocated_bytes);
+    const jlong current_live_set = atomic_add_jlong((jlong)alloc_size, &_live_set_bytes);
     log_trace(jfr, system)("Allocation: [" SIZE_FORMAT "] bytes", alloc_size);
     log_trace(jfr, system)("Total alloc [" JLONG_FORMAT "] bytes", total_allocated);
     log_trace(jfr, system)("Liveset:    [" JLONG_FORMAT "] bytes", current_live_set);
@@ -63,11 +63,11 @@ static void add(size_t alloc_size) {
 
 static void subtract(size_t dealloc_size) {
   if (!JfrRecorder::is_created()) {
-    const size_t total_deallocated = atomic_add_jlong(dealloc_size, &_deallocated_bytes);
-    const size_t current_live_set = atomic_add_jlong(dealloc_size * -1, &_live_set_bytes);
+    const jlong total_deallocated = atomic_add_jlong((jlong)dealloc_size, &_deallocated_bytes);
+    const jlong current_live_set = atomic_add_jlong(((jlong)dealloc_size * -1), &_live_set_bytes);
     log_trace(jfr, system)("Deallocation: [" SIZE_FORMAT "] bytes", dealloc_size);
-    log_trace(jfr, system)("Total dealloc [" SIZE_FORMAT "] bytes", total_deallocated);
-    log_trace(jfr, system)("Liveset:      [" SIZE_FORMAT "] bytes", current_live_set);
+    log_trace(jfr, system)("Total dealloc [" JLONG_FORMAT "] bytes", total_deallocated);
+    log_trace(jfr, system)("Liveset:      [" JLONG_FORMAT "] bytes", current_live_set);
   }
 }