8309862: Unsafe list operations in JfrStringPool

Reviewed-by: egahlin

Author: Markus Grönlund

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

@@ -338,18 +338,20 @@ static size_t write_storage(JfrStorage& storage, JfrChunkWriter& chunkwriter) {
   return invoke(fs);
 }
 
-typedef Content<JfrStringPool, &JfrStringPool::write> StringPool;
-typedef WriteCheckpointEvent<StringPool> WriteStringPool;
+typedef Content<JfrStringPool, &JfrStringPool::flush> FlushStringPoolFunctor;
+typedef Content<JfrStringPool, &JfrStringPool::write> WriteStringPoolFunctor;
+typedef WriteCheckpointEvent<FlushStringPoolFunctor> FlushStringPool;
+typedef WriteCheckpointEvent<WriteStringPoolFunctor> WriteStringPool;
 
 static u4 flush_stringpool(JfrStringPool& string_pool, JfrChunkWriter& chunkwriter) {
-  StringPool sp(string_pool);
-  WriteStringPool wsp(chunkwriter, sp, TYPE_STRING);
-  return invoke(wsp);
+  FlushStringPoolFunctor fspf(string_pool);
+  FlushStringPool fsp(chunkwriter, fspf, TYPE_STRING);
+  return invoke(fsp);
 }
 
 static u4 write_stringpool(JfrStringPool& string_pool, JfrChunkWriter& chunkwriter) {
-  StringPool sp(string_pool);
-  WriteStringPool wsp(chunkwriter, sp, TYPE_STRING);
+  WriteStringPoolFunctor wspf(string_pool);
+  WriteStringPool wsp(chunkwriter, wspf, TYPE_STRING);
   return invoke(wsp);
 }
 
@@ -461,7 +463,6 @@ void JfrRecorderService::clear() {
 
 void JfrRecorderService::pre_safepoint_clear() {
   clear_object_allocation_sampling();
-  _string_pool.clear();
   _storage.clear();
   JfrStackTraceRepository::clear();
 }
@@ -475,14 +476,14 @@ void JfrRecorderService::invoke_safepoint_clear() {
 void JfrRecorderService::safepoint_clear() {
   assert(SafepointSynchronize::is_at_safepoint(), "invariant");
   _checkpoint_manager.begin_epoch_shift();
-  _string_pool.clear();
   _storage.clear();
   _chunkwriter.set_time_stamp();
   JfrStackTraceRepository::clear();
   _checkpoint_manager.end_epoch_shift();
 }
 
 void JfrRecorderService::post_safepoint_clear() {
+  _string_pool.clear();
   _checkpoint_manager.clear();
 }
 
@@ -567,9 +568,6 @@ void JfrRecorderService::pre_safepoint_write() {
     // The sampler is released (unlocked) later in post_safepoint_write.
     ObjectSampleCheckpoint::on_rotation(ObjectSampler::acquire());
   }
-  if (_string_pool.is_modified()) {
-    write_stringpool(_string_pool, _chunkwriter);
-  }
   write_storage(_storage, _chunkwriter);
   if (_stack_trace_repository.is_modified()) {
     write_stacktrace(_stack_trace_repository, _chunkwriter, false);
@@ -587,9 +585,6 @@ void JfrRecorderService::safepoint_write() {
   assert(SafepointSynchronize::is_at_safepoint(), "invariant");
   _checkpoint_manager.begin_epoch_shift();
   JfrStackTraceRepository::clear_leak_profiler();
-  if (_string_pool.is_modified()) {
-    write_stringpool(_string_pool, _chunkwriter);
-  }
   _checkpoint_manager.on_rotation();
   _storage.write_at_safepoint();
   _chunkwriter.set_time_stamp();
@@ -603,6 +598,7 @@ void JfrRecorderService::post_safepoint_write() {
   // Type tagging is epoch relative which entails we are able to write out the
   // already tagged artifacts for the previous epoch. We can accomplish this concurrently
   // with threads now tagging artifacts in relation to the new, now updated, epoch and remain outside of a safepoint.
+  write_stringpool(_string_pool, _chunkwriter);
   _checkpoint_manager.write_type_set();
   if (LeakProfiler::is_running()) {
     // The object sampler instance was exclusively acquired and locked in pre_safepoint_write.

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

@@ -208,4 +208,11 @@ class EpochDispatchOp {
   size_t elements() const { return _elements; }
 };
 
+template <typename T>
+class ReinitializationOp {
+ public:
+  typedef T Type;
+  bool process(Type* t);
+};
+
 #endif // SHARE_JFR_RECORDER_STORAGE_JFRSTORAGEUTILS_HPP

src/hotspot/share/jfr/recorder/storage/jfrStorageUtils.inline.hpp

@@ -175,4 +175,13 @@ size_t EpochDispatchOp<Operation>::dispatch(bool previous_epoch, const u1* eleme
   return elements;
 }
 
+template <typename T>
+bool ReinitializationOp<T>::process(T* t) {
+  assert(t != nullptr, "invariant");
+  assert(t->identity() != nullptr, "invariant");
+  t->reinitialize();
+  t->release();
+  return true;
+}
+
 #endif // SHARE_JFR_RECORDER_STORAGE_JFRSTORAGEUTILS_INLINE_HPP

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

@@ -77,10 +77,17 @@ static const size_t string_pool_buffer_size = 512 * K;
 bool JfrStringPool::initialize() {
   assert(_mspace == nullptr, "invariant");
   _mspace = create_mspace<JfrStringPoolMspace>(string_pool_buffer_size,
-                                               string_pool_cache_count, // cache limit
-                                               string_pool_cache_count, // cache preallocate count
-                                               false, // preallocate_to_free_list (== preallocate directly to live list)
+                                               0,
+                                               0, // cache preallocate count
+                                               false,
                                                this);
+
+  // preallocate buffer count to each of the epoch live lists
+  for (size_t i = 0; i < string_pool_cache_count * 2; ++i) {
+    Buffer* const buffer = mspace_allocate(string_pool_buffer_size, _mspace);
+    _mspace->add_to_live_list(buffer, i % 2 == 0);
+  }
+  assert(_mspace->free_list_is_empty(), "invariant");
   return _mspace != nullptr;
 }
 
@@ -95,11 +102,7 @@ static void release(BufferPtr buffer, Thread* thread) {
   assert(buffer->lease(), "invariant");
   assert(buffer->acquired_by_self(), "invariant");
   buffer->clear_lease();
-  if (buffer->transient()) {
-    buffer->set_retired();
-  } else {
-    buffer->release();
-  }
+  buffer->release();
 }
 
 BufferPtr JfrStringPool::flush(BufferPtr old, size_t used, size_t requested, Thread* thread) {
@@ -180,30 +183,44 @@ typedef StringPoolOp<UnBufferedWriteToChunk> WriteOperation;
 typedef StringPoolOp<StringPoolDiscarderStub> DiscardOperation;
 typedef ExclusiveOp<WriteOperation> ExclusiveWriteOperation;
 typedef ExclusiveOp<DiscardOperation> ExclusiveDiscardOperation;
+typedef ReinitializationOp<JfrStringPoolBuffer> ReinitializationOperation;
 typedef ReleaseWithExcisionOp<JfrStringPoolMspace, JfrStringPoolMspace::LiveList> ReleaseOperation;
 typedef CompositeOperation<ExclusiveWriteOperation, ReleaseOperation> WriteReleaseOperation;
+typedef CompositeOperation<ExclusiveWriteOperation, ReinitializationOperation> WriteReinitializeOperation;
 typedef CompositeOperation<ExclusiveDiscardOperation, ReleaseOperation> DiscardReleaseOperation;
 
 size_t JfrStringPool::write() {
   Thread* const thread = Thread::current();
   WriteOperation wo(_chunkwriter, thread);
   ExclusiveWriteOperation ewo(wo);
   assert(_mspace->free_list_is_empty(), "invariant");
-  ReleaseOperation ro(_mspace, _mspace->live_list());
+  ReleaseOperation ro(_mspace, _mspace->live_list(true)); // previous epoch list
   WriteReleaseOperation wro(&ewo, &ro);
   assert(_mspace->live_list_is_nonempty(), "invariant");
-  process_live_list(wro, _mspace);
+  process_live_list(wro, _mspace, true); // previous epoch list
+  return wo.processed();
+}
+
+size_t JfrStringPool::flush() {
+  Thread* const thread = Thread::current();
+  WriteOperation wo(_chunkwriter, thread);
+  ExclusiveWriteOperation ewo(wo);
+  ReinitializationOperation rio;
+  WriteReinitializeOperation wro(&ewo, &rio);
+  assert(_mspace->free_list_is_empty(), "invariant");
+  assert(_mspace->live_list_is_nonempty(), "invariant");
+  process_live_list(wro, _mspace); // current epoch list
   return wo.processed();
 }
 
 size_t JfrStringPool::clear() {
   DiscardOperation discard_operation;
   ExclusiveDiscardOperation edo(discard_operation);
   assert(_mspace->free_list_is_empty(), "invariant");
-  ReleaseOperation ro(_mspace, _mspace->live_list());
+  ReleaseOperation ro(_mspace, _mspace->live_list(true)); // previous epoch list
   DiscardReleaseOperation discard_op(&edo, &ro);
   assert(_mspace->live_list_is_nonempty(), "invariant");
-  process_live_list(discard_op, _mspace);
+  process_live_list(discard_op, _mspace, true); // previous epoch list
   return discard_operation.processed();
 }
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2023, 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
@@ -35,7 +35,7 @@ class JavaThread;
 class JfrChunkWriter;
 class JfrStringPool;
 
-typedef JfrMemorySpace<JfrStringPool, JfrMspaceRetrieval, JfrLinkedList<JfrStringPoolBuffer> > JfrStringPoolMspace;
+typedef JfrMemorySpace<JfrStringPool, JfrMspaceRetrieval, JfrLinkedList<JfrStringPoolBuffer>, JfrLinkedList<JfrStringPoolBuffer>, true > JfrStringPoolMspace;
 
 //
 // Although called JfrStringPool, a more succinct description would be
@@ -45,8 +45,10 @@ typedef JfrMemorySpace<JfrStringPool, JfrMspaceRetrieval, JfrLinkedList<JfrStrin
 //
 class JfrStringPool : public JfrCHeapObj {
  public:
-  size_t write();
   size_t clear();
+  size_t flush();
+  size_t write();
+
   static jboolean add(jlong id, jstring string, JavaThread* jt);
 
   typedef JfrStringPoolMspace::Node    Buffer;