8257621: JFR StringPool misses cached items across consecutive recordings

Reviewed-by: egahlin

Author: Markus Grönlund

src/hotspot/share/jfr/jni/jfrJniMethod.cpp

@@ -160,10 +160,6 @@ NO_TRANSITION(jboolean, jfr_is_available(JNIEnv* env, jclass jvm))
   return !Jfr::is_disabled() ? JNI_TRUE : JNI_FALSE;
 NO_TRANSITION_END
 
-NO_TRANSITION(jlong, jfr_get_epoch_address(JNIEnv* env, jobject jvm))
-  return JfrTraceIdEpoch::epoch_address();
-NO_TRANSITION_END
-
 NO_TRANSITION(jlong, jfr_get_unloaded_event_classes_count(JNIEnv* env, jobject jvm))
   return JfrKlassUnloading::event_class_count();
 NO_TRANSITION_END
@@ -327,8 +323,8 @@ JVM_ENTRY_NO_ENV(jlong, jfr_type_id(JNIEnv* env, jobject jvm, jclass jc))
   return JfrTraceId::load_raw(jc);
 JVM_END
 
-JVM_ENTRY_NO_ENV(jboolean, jfr_add_string_constant(JNIEnv* env, jclass jvm, jboolean epoch, jlong id, jstring string))
-  return JfrStringPool::add(epoch == JNI_TRUE, id, string, thread) ? JNI_TRUE : JNI_FALSE;
+JVM_ENTRY_NO_ENV(jboolean, jfr_add_string_constant(JNIEnv* env, jclass jvm, jlong id, jstring string))
+  return JfrStringPool::add(id, string, thread);
 JVM_END
 
 JVM_ENTRY_NO_ENV(void, jfr_set_force_instrumentation(JNIEnv* env, jobject jvm, jboolean force_instrumentation))

src/hotspot/share/jfr/jni/jfrJniMethod.hpp

@@ -120,9 +120,7 @@ jboolean JNICALL jfr_event_writer_flush(JNIEnv* env, jclass cls, jobject writer,
 void JNICALL jfr_flush(JNIEnv* env, jobject jvm);
 void JNICALL jfr_abort(JNIEnv* env, jobject jvm, jstring errorMsg);
 
-jlong JNICALL jfr_get_epoch_address(JNIEnv* env, jobject jvm);
-
-jboolean JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, jboolean epoch, jlong id, jstring string);
+jboolean JNICALL jfr_add_string_constant(JNIEnv* env, jclass jvm, jlong id, jstring string);
 
 void JNICALL jfr_uncaught_exception(JNIEnv* env, jobject jvm, jobject thread, jthrowable throwable);
 

src/hotspot/share/jfr/jni/jfrJniMethodRegistration.cpp

@@ -75,8 +75,7 @@ JfrJniMethodRegistration::JfrJniMethodRegistration(JNIEnv* env) {
       (char*)"flush", (char*)"()V", (void*)jfr_flush,
       (char*)"setRepositoryLocation", (char*)"(Ljava/lang/String;)V", (void*)jfr_set_repository_location,
       (char*)"abort", (char*)"(Ljava/lang/String;)V", (void*)jfr_abort,
-      (char*)"getEpochAddress", (char*)"()J",(void*)jfr_get_epoch_address,
-      (char*)"addStringConstant", (char*)"(ZJLjava/lang/String;)Z", (void*)jfr_add_string_constant,
+      (char*)"addStringConstant", (char*)"(JLjava/lang/String;)Z", (void*)jfr_add_string_constant,
       (char*)"uncaughtException", (char*)"(Ljava/lang/Thread;Ljava/lang/Throwable;)V", (void*)jfr_uncaught_exception,
       (char*)"setForceInstrumentation", (char*)"(Z)V", (void*)jfr_set_force_instrumentation,
       (char*)"getUnloadedEventClassCount", (char*)"()J", (void*)jfr_get_unloaded_event_classes_count,

src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp

@@ -42,6 +42,7 @@
 #include "jfr/utilities/jfrBigEndian.hpp"
 #include "jfr/utilities/jfrIterator.hpp"
 #include "jfr/utilities/jfrLinkedList.inline.hpp"
+#include "jfr/utilities/jfrSignal.hpp"
 #include "jfr/utilities/jfrThreadIterator.hpp"
 #include "jfr/utilities/jfrTypes.hpp"
 #include "jfr/writers/jfrJavaEventWriter.hpp"
@@ -57,22 +58,7 @@
 
 typedef JfrCheckpointManager::BufferPtr BufferPtr;
 
-static volatile bool constant_pending = false;
-
-static bool is_constant_pending() {
-  if (Atomic::load_acquire(&constant_pending)) {
-    Atomic::release_store(&constant_pending, false); // reset
-    return true;
-  }
-  return false;
-}
-
-static void set_constant_pending() {
-  if (!Atomic::load_acquire(&constant_pending)) {
-    Atomic::release_store(&constant_pending, true);
-  }
-}
-
+static JfrSignal _new_checkpoint;
 static JfrCheckpointManager* _instance = NULL;
 
 JfrCheckpointManager& JfrCheckpointManager::instance() {
@@ -231,7 +217,7 @@ BufferPtr JfrCheckpointManager::flush(BufferPtr old, size_t used, size_t request
     // indicates a lease is being returned
     release(old);
     // signal completion of a new checkpoint
-    set_constant_pending();
+    _new_checkpoint.signal();
     return NULL;
   }
   BufferPtr new_buffer = lease(old, thread, used + requested);
@@ -474,7 +460,7 @@ size_t JfrCheckpointManager::flush_type_set() {
       elements = ::flush_type_set(thread);
     }
   }
-  if (is_constant_pending()) {
+  if (_new_checkpoint.is_signaled()) {
     WriteOperation wo(_chunkwriter);
     MutexedWriteOperation mwo(wo);
     _thread_local_mspace->iterate(mwo); // current epoch list

src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.cpp

@@ -26,9 +26,9 @@
 #include "jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp"
 #include "runtime/safepoint.hpp"
 
+JfrSignal JfrTraceIdEpoch::_tag_state;
 bool JfrTraceIdEpoch::_epoch_state = false;
 bool JfrTraceIdEpoch::_synchronizing = false;
-volatile bool JfrTraceIdEpoch::_changed_tag_state = false;
 
 void JfrTraceIdEpoch::begin_epoch_shift() {
   assert(SafepointSynchronize::is_at_safepoint(), "invariant");
@@ -43,3 +43,4 @@ void JfrTraceIdEpoch::end_epoch_shift() {
   OrderAccess::storestore();
   _synchronizing = false;
 }
+

src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdEpoch.hpp

@@ -25,6 +25,7 @@
 #ifndef SHARE_JFR_RECORDER_CHECKPOINT_TYPES_TRACEID_JFRTRACEIDEPOCH_HPP
 #define SHARE_JFR_RECORDER_CHECKPOINT_TYPES_TRACEID_JFRTRACEIDEPOCH_HPP
 
+#include "jfr/utilities/jfrSignal.hpp"
 #include "jfr/utilities/jfrTypes.hpp"
 #include "memory/allocation.hpp"
 #include "runtime/atomic.hpp"
@@ -54,21 +55,13 @@
 class JfrTraceIdEpoch : AllStatic {
   friend class JfrCheckpointManager;
  private:
+  static JfrSignal _tag_state;
   static bool _epoch_state;
   static bool _synchronizing;
-  static volatile bool _changed_tag_state;
 
   static void begin_epoch_shift();
   static void end_epoch_shift();
 
-  static bool changed_tag_state() {
-    return Atomic::load_acquire(&_changed_tag_state);
-  }
-
-  static void set_tag_state(bool value) {
-    Atomic::release_store(&_changed_tag_state, value);
-  }
-
  public:
   static bool epoch() {
     return _epoch_state;
@@ -115,17 +108,11 @@ class JfrTraceIdEpoch : AllStatic {
   }
 
   static bool has_changed_tag_state() {
-    if (changed_tag_state()) {
-      set_tag_state(false);
-      return true;
-    }
-    return false;
+    return _tag_state.is_signaled();
   }
 
   static void set_changed_tag_state() {
-    if (!changed_tag_state()) {
-      set_tag_state(true);
-    }
+    _tag_state.signal();
   }
 };
 

src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp

@@ -314,9 +314,7 @@ static size_t write_storage(JfrStorage& storage, JfrChunkWriter& chunkwriter) {
 }
 
 typedef Content<JfrStringPool, &JfrStringPool::write> StringPool;
-typedef Content<JfrStringPool, &JfrStringPool::write_at_safepoint> StringPoolSafepoint;
 typedef WriteCheckpointEvent<StringPool> WriteStringPool;
-typedef WriteCheckpointEvent<StringPoolSafepoint> WriteStringPoolSafepoint;
 
 static u4 flush_stringpool(JfrStringPool& string_pool, JfrChunkWriter& chunkwriter) {
   StringPool sp(string_pool);
@@ -330,12 +328,6 @@ static u4 write_stringpool(JfrStringPool& string_pool, JfrChunkWriter& chunkwrit
   return invoke(wsp);
 }
 
-static u4 write_stringpool_safepoint(JfrStringPool& string_pool, JfrChunkWriter& chunkwriter) {
-  StringPoolSafepoint sps(string_pool);
-  WriteStringPoolSafepoint wsps(chunkwriter, sps, TYPE_STRING);
-  return invoke(wsps);
-}
-
 typedef Content<JfrCheckpointManager, &JfrCheckpointManager::flush_type_set> FlushTypeSetFunctor;
 typedef WriteContent<FlushTypeSetFunctor> FlushTypeSet;
 
@@ -569,7 +561,7 @@ void JfrRecorderService::safepoint_write() {
   assert(SafepointSynchronize::is_at_safepoint(), "invariant");
   _checkpoint_manager.begin_epoch_shift();
   if (_string_pool.is_modified()) {
-    write_stringpool_safepoint(_string_pool, _chunkwriter);
+    write_stringpool(_string_pool, _chunkwriter);
   }
   _checkpoint_manager.on_rotation();
   _storage.write_at_safepoint();

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

@@ -32,6 +32,7 @@
 #include "jfr/recorder/stringpool/jfrStringPool.hpp"
 #include "jfr/recorder/stringpool/jfrStringPoolWriter.hpp"
 #include "jfr/utilities/jfrLinkedList.inline.hpp"
+#include "jfr/utilities/jfrSignal.hpp"
 #include "jfr/utilities/jfrTypes.hpp"
 #include "logging/log.hpp"
 #include "runtime/atomic.hpp"
@@ -40,44 +41,19 @@
 
 typedef JfrStringPool::BufferPtr BufferPtr;
 
-static JfrStringPool* _instance = NULL;
-static uint64_t store_generation = 0;
-static uint64_t serialized_generation = 0;
-
-inline void set_generation(uint64_t value, uint64_t* const dest) {
-  assert(dest != NULL, "invariant");
-  Atomic::release_store(dest, value);
-}
-
-static void increment_store_generation() {
-  const uint64_t current_serialized = Atomic::load_acquire(&serialized_generation);
-  const uint64_t current_stored = Atomic::load_acquire(&store_generation);
-  if (current_serialized == current_stored) {
-    set_generation(current_serialized + 1, &store_generation);
-  }
-}
-
-static bool increment_serialized_generation() {
-  const uint64_t current_stored = Atomic::load_acquire(&store_generation);
-  const uint64_t current_serialized = Atomic::load_acquire(&serialized_generation);
-  if (current_stored != current_serialized) {
-    set_generation(current_stored, &serialized_generation);
-    return true;
-  }
-  return false;
-}
+static JfrSignal _new_string;
 
 bool JfrStringPool::is_modified() {
-  return increment_serialized_generation();
+  return _new_string.is_signaled();
 }
 
+static JfrStringPool* _instance = NULL;
+
 JfrStringPool& JfrStringPool::instance() {
   return *_instance;
 }
 
 JfrStringPool* JfrStringPool::create(JfrChunkWriter& cw) {
-  store_generation = 0;
-  serialized_generation = 0;
   assert(_instance == NULL, "invariant");
   _instance = new JfrStringPool(cw);
   return _instance;
@@ -155,20 +131,16 @@ BufferPtr JfrStringPool::lease(Thread* thread, size_t size /* 0 */) {
   return buffer;
 }
 
-bool JfrStringPool::add(bool epoch, jlong id, jstring string, JavaThread* jt) {
+jboolean JfrStringPool::add(jlong id, jstring string, JavaThread* jt) {
   assert(jt != NULL, "invariant");
-  const bool current_epoch = JfrTraceIdEpoch::epoch();
-  if (current_epoch != epoch) {
-    return current_epoch;
-  }
   {
     JfrStringPoolWriter writer(jt);
     writer.write(id);
     writer.write(string);
     writer.inc_nof_strings();
   }
-  increment_store_generation();
-  return current_epoch;
+  _new_string.signal();
+  return JNI_TRUE;
 }
 
 template <template <typename> class Operation>
@@ -224,13 +196,7 @@ size_t JfrStringPool::write() {
   return wo.processed();
 }
 
-size_t JfrStringPool::write_at_safepoint() {
-  assert(SafepointSynchronize::is_at_safepoint(), "invariant");
-  return write();
-}
-
 size_t JfrStringPool::clear() {
-  increment_serialized_generation();
   DiscardOperation discard_operation;
   ExclusiveDiscardOperation edo(discard_operation);
   assert(_mspace->free_list_is_empty(), "invariant");

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

@@ -45,10 +45,10 @@ typedef JfrMemorySpace<JfrStringPool, JfrMspaceRetrieval, JfrLinkedList<JfrStrin
 //
 class JfrStringPool : public JfrCHeapObj {
  public:
-  static bool add(bool epoch, jlong id, jstring string, JavaThread* jt);
   size_t write();
-  size_t write_at_safepoint();
   size_t clear();
+  static jboolean add(jlong id, jstring string, JavaThread* jt);
+
   typedef JfrStringPoolMspace::Node    Buffer;
   typedef JfrStringPoolMspace::NodePtr BufferPtr;
 

src/hotspot/share/jfr/utilities/jfrSignal.hpp

@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#ifndef SHARE_JFR_UTILITIES_JFRSIGNAL_HPP
+#define SHARE_JFR_UTILITIES_JFRSIGNAL_HPP
+
+#include "runtime/atomic.hpp"
+
+class JfrSignal {
+ private:
+  mutable volatile bool _signaled;
+ public:
+  JfrSignal() : _signaled(false) {}
+
+  void signal() const {
+    if (!Atomic::load_acquire(&_signaled)) {
+      Atomic::release_store(&_signaled, true);
+    }
+  }
+
+  bool is_signaled() const {
+    if (Atomic::load_acquire(&_signaled)) {
+      Atomic::release_store(&_signaled, false); // auto-reset
+      return true;
+    }
+    return false;
+  }
+};
+
+#endif // SHARE_JFR_UTILITIES_JFRSIGNAL_HPP