8259036: Failed JfrVersionSystem invariant when VM built with -fno-elide-constructors

Reviewed-by: egahlin

src/hotspot/share/jfr/utilities/jfrConcurrentLinkedListHost.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021 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
@@ -47,7 +47,7 @@ inline Node* mark_for_removal(Node* node) {
 
 /*
  * The insertion marker (i.e. the insertion bit) is represented by '[ ]' as part of state description comments:
- * "node --> next" becomes "[node] --> next", in an attempt to convey node as being exlusively reserved.
+ * "node --> next" becomes "[node] --> next", in an attempt to convey the node as exlusively reserved.
  */
 template <typename Node>
 inline bool mark_for_insertion(Node* node, const Node* tail) {
@@ -66,8 +66,7 @@ Node* find_adjacent(Node* head, const Node* tail, Node** predecessor, VersionHan
   Node* predecessor_next = NULL;
   while (true) {
     Node* current = head;
-    version_handle.checkout();
-    assert(version_handle.is_tracked(), "invariant");
+    version_handle->checkout();
     Node* next = Atomic::load_acquire(&current->_next);
     do {
       assert(next != NULL, "invariant");
@@ -117,7 +116,6 @@ void JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPolicy>::insert_head
   while (true) {
     // Find an adjacent predecessor and successor node pair.
     successor = find_adjacent<Node, VersionHandle, HeadNode>(head, tail, &predecessor, version_handle, predicate);
-    assert(version_handle.is_tracked(), "invariant");
     // Invariant (adjacency): predecessor --> successor
     // Invariant (optional: key-based total order): predecessor->key() < key  && key <= successor->key().
     // We can now attempt to insert the new node in-between.
@@ -149,7 +147,6 @@ void JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPolicy>::insert_tail
   while (true) {
     // Find an adjacent predecessor and successor node pair, where the successor == tail
     const NodePtr successor = find_adjacent<Node, VersionHandle, LastNode>(last, tail, &predecessor, version_handle, predicate);
-    assert(version_handle.is_tracked(), "invariant");
     assert(successor == tail, "invariant");
     // Invariant: predecessor --> successor
     // We first attempt to mark the predecessor node to signal our intent of performing an insertion.
@@ -182,7 +179,7 @@ void JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPolicy>::insert_tail
     head->_next = node;
     // Invariant: head --> [node] --> tail
   }
-  version_handle.release(); // release_store_fence
+  OrderAccess::storestore();
   // Publish the inserted node by removing the insertion marker.
   node->_next = const_cast<NodePtr>(tail);
   // Invariant: last --> node --> tail (possibly also head --> node --> tail)
@@ -204,7 +201,6 @@ typename Client::Node* JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPo
   while (true) {
     // Find an adjacent predecessor and successor node pair.
     successor = find_adjacent<Node, VersionHandle, SearchPolicy>(head, tail, &predecessor, version_handle, predicate);
-    assert(version_handle.is_tracked(), "invariant");
     if (successor == tail) {
       return NULL;
     }
@@ -228,7 +224,6 @@ typename Client::Node* JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPo
     // Physically excise using slow path, can be completed asynchronously by other threads.
     Identity<Node> excise(successor);
     find_adjacent<Node, VersionHandle, Identity>(head, tail, &predecessor, version_handle, excise);
-    assert(version_handle.is_tracked(), "invariant");
   }
   if (last != NULL && Atomic::load_acquire(&last->_next) == successor) {
     guarantee(!insert_is_head, "invariant");
@@ -237,11 +232,9 @@ typename Client::Node* JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPo
     find_adjacent<Node, VersionHandle, LastNode>(last, tail, &predecessor, version_handle, excise);
     // Invariant: successor excised from last list
   }
-  // Increment the current version so we can track when other threads have seen this update.
-  VersionType version = version_handle.increment();
-  version_handle.release(); // release_store_fence
-  // Rendezvous with checkouts for versions less than this version.
-  version_handle.await(version);
+  // Commit the modification back to the version control system.
+  // It blocks until all checkouts for versions earlier than the commit have been released.
+  version_handle->commit();
   // At this point we know there can be no references onto the excised node. It is safe, enjoy it.
   return successor;
 }
@@ -255,8 +248,7 @@ bool JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPolicy>::in_list(con
   assert(head != tail, "invariant");
   VersionHandle version_handle = _client->get_version_handle();
   const Node* current = head;
-  version_handle.checkout();
-  assert(version_handle.is_tracked(), "invariant");
+  version_handle->checkout();
   const Node* next = Atomic::load_acquire(&current->_next);
   while (true) {
     if (!is_marked_for_removal(next)) {
@@ -281,8 +273,7 @@ inline void JfrConcurrentLinkedListHost<Client, SearchPolicy, AllocPolicy>::iter
   assert(head != tail, "invariant");
   VersionHandle version_handle = _client->get_version_handle();
   NodePtr current = head;
-  version_handle.checkout();
-  assert(version_handle.is_tracked(), "invariant");
+  version_handle->checkout();
   NodePtr next = Atomic::load_acquire(&current->_next);
   while (true) {
     if (!is_marked_for_removal(next)) {

src/hotspot/share/jfr/utilities/jfrConcurrentQueue.inline.hpp

@@ -70,7 +70,7 @@ void JfrConcurrentQueue<NodeType, AllocPolicy>::iterate(Callback& cb) {
 
 template <typename NodeType, typename AllocPolicy>
 inline JfrVersionSystem::Handle JfrConcurrentQueue<NodeType, AllocPolicy>::get_version_handle() {
-  return _version_system.get_handle();
+  return _version_system.get();
 }
 
 template <typename NodeType, typename AllocPolicy>

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, 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
@@ -30,8 +30,6 @@
 
 template <typename T>
 class RefCountHandle {
-  template <typename, typename>
-  friend class RefCountPointer;
  private:
   const T* _ptr;
 
@@ -51,9 +49,8 @@ class RefCountHandle {
 
   ~RefCountHandle() {
     if (_ptr != NULL) {
-      const T* temp = _ptr;
+      _ptr->remove_ref();
       _ptr = NULL;
-      temp->remove_ref();
     }
   }
 
@@ -76,18 +73,41 @@ class RefCountHandle {
     return _ptr != NULL;
   }
 
-  const T & operator->() const {
+  const T& operator->() const {
     return *_ptr;
   }
 
   T& operator->() {
     return *const_cast<T*>(_ptr);
   }
+
+  static RefCountHandle<T> make(const T* ptr) {
+    return ptr;
+  }
+};
+
+class SingleThreadedRefCounter {
+ private:
+  mutable intptr_t _refs;
+ public:
+  SingleThreadedRefCounter() : _refs(0) {}
+
+  void inc() const {
+    ++_refs;
+  }
+
+  bool dec() const {
+    return --_refs == 0;
+  }
+
+  intptr_t current() const {
+    return _refs;
+  }
 };
 
 class MultiThreadedRefCounter {
  private:
-  mutable volatile int _refs;
+  mutable volatile intptr_t _refs;
  public:
   MultiThreadedRefCounter() : _refs(0) {}
 
@@ -99,7 +119,7 @@ class MultiThreadedRefCounter {
     return 0 == Atomic::add(&_refs, (-1));
   }
 
-  int current() const {
+  intptr_t current() const {
    return _refs;
   }
 };
@@ -146,8 +166,7 @@ class RefCountPointer : public JfrCHeapObj {
   }
 
   static RefHandle make(const T* ptr) {
-    assert(ptr != NULL, "invariant");
-    return RefHandle(new RefCountPointer<T, RefCountImpl>(ptr));
+    return RefHandle::make(new RefCountPointer<T, RefCountImpl>(ptr));
   }
 };
 

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021 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
@@ -58,6 +58,7 @@
  */
 
 #include "jfr/utilities/jfrAllocation.hpp"
+#include "jfr/utilities/jfrRefCountPointer.hpp"
 #include "jfr/utilities/jfrTypes.hpp"
 #include "memory/padded.hpp"
 
@@ -66,41 +67,43 @@ class JfrVersionSystem : public JfrCHeapObj {
   typedef traceid Type;
  private:
   class Node : public JfrCHeapObj {
-   public:
+    friend class JfrVersionSystem;
+    template <typename>
+    friend class RefCountHandle;
+   private:
+    JfrVersionSystem* const _system;
     Node* _next;
-    Type _version;
-    bool _live;
-    Node();
+    mutable Type _version;
+    SingleThreadedRefCounter _ref_counter;
+    mutable bool _live;
+    Node(JfrVersionSystem* system);
+    void add_ref() const;
+    void remove_ref() const;
     Type version() const;
-    void set(Type version);
+    void set(Type version) const;
+   public:
+    void checkout();
+    void commit();
+    const Node* operator->() const { return this; }
+    Node* operator->() { return this; }
   };
   typedef Node* NodePtr;
- public:
-  class Handle {
-   private:
-    JfrVersionSystem* _system;
-    NodePtr _node;
-    Handle(JfrVersionSystem* system);
-   public:
-    Handle();
-    ~Handle();
-    void checkout();
-    void release();
-    Type increment();
-    void await(Type version);
-    DEBUG_ONLY(bool is_tracked() const;)
-    friend class JfrVersionSystem;
-  };
 
+ public:
   JfrVersionSystem();
   ~JfrVersionSystem();
   void reset();
 
-  // to access the versioning system
-  Handle get_handle();
-  Handle checkout_handle();
+  typedef RefCountHandle<Node> Handle;
+  Handle get();
 
  private:
+  NodePtr acquire();
+  void await(Type version);
+  Type tip() const;
+  Type inc_tip();
+  NodePtr synchronize_with(Type version, NodePtr last) const;
+  DEBUG_ONLY(void assert_state(const Node* node) const;)
   struct PaddedTip {
     DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE, 0);
     volatile Type _value;
@@ -108,16 +111,6 @@ class JfrVersionSystem : public JfrCHeapObj {
   };
   PaddedTip _tip;
   NodePtr _head;
-  volatile int _spinlock;
-
-  NodePtr acquire();
-  void release(NodePtr node);
-  void await(Type version);
-  Type tip() const;
-  Type increment();
-  NodePtr synchronize_with(Type version, NodePtr last) const;
-  debug_only(bool is_registered(Type version) const;)
-  friend class Handle;
 };
 
 #endif // SHARE_JFR_UTILITIES_JFRVERSIONSYSTEM_HPP

src/hotspot/share/jfr/utilities/jfrVersionSystem.inline.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2021 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
@@ -29,17 +29,7 @@
 #include "runtime/atomic.hpp"
 #include "runtime/os.inline.hpp"
 
-inline JfrVersionSystem::Node::Node() : _next(NULL), _version(0), _live(true) {}
-
-inline traceid JfrVersionSystem::Node::version() const {
-  return _version;
-}
-
-inline void JfrVersionSystem::Node::set(traceid version) {
-  Atomic::release_store_fence(&_version, version);
-}
-
-inline JfrVersionSystem::JfrVersionSystem() : _tip(), _head(NULL), _spinlock(0) {
+inline JfrVersionSystem::JfrVersionSystem() : _tip(), _head(NULL) {
   _tip._value = 1;
 }
 
@@ -62,7 +52,7 @@ inline JfrVersionSystem::Type JfrVersionSystem::tip() const {
   return Atomic::load(&_tip._value);
 }
 
-inline JfrVersionSystem::Type JfrVersionSystem::increment() {
+inline JfrVersionSystem::Type JfrVersionSystem::inc_tip() {
   traceid cmp;
   traceid xchg;
   do {
@@ -80,24 +70,59 @@ inline JfrVersionSystem::NodePtr JfrVersionSystem::acquire() {
       node = node->_next;
       continue;
     }
-    assert(node->_version == 0, "invariant");
+    DEBUG_ONLY(assert_state(node);)
     return node;
   }
   // new
-  node = new Node();
+  node = new Node(this);
   NodePtr next;
   do {
     next = _head;
     node->_next = next;
   } while (Atomic::cmpxchg(&_head, next, node) != next);
+  DEBUG_ONLY(assert_state(node);)
   return node;
 }
 
-inline void JfrVersionSystem::release(JfrVersionSystem::NodePtr node) {
-  assert(node != NULL, "invariant");
-  assert(node->_live, "invariant");
-  Atomic::release_store_fence(&node->_version, (traceid)0);
-  node->_live = false;
+inline JfrVersionSystem::Handle JfrVersionSystem::get() {
+  return Handle::make(acquire());
+}
+
+inline JfrVersionSystem::Node::Node(JfrVersionSystem* system) : _system(system), _next(NULL), _version(0), _live(true) {}
+
+inline traceid JfrVersionSystem::Node::version() const {
+  return _version;
+}
+
+inline void JfrVersionSystem::Node::set(traceid version) const {
+  Atomic::release_store_fence(&_version, version);
+}
+
+inline void JfrVersionSystem::Node::add_ref() const {
+  _ref_counter.inc();
+}
+
+inline void JfrVersionSystem::Node::remove_ref() const {
+  if (_ref_counter.dec()) {
+    assert(_live, "invariant");
+    set(0);
+    _live = false;
+  }
+}
+
+inline void JfrVersionSystem::Node::checkout() {
+  set(_system->tip());
+  assert(version() != 0, "invariant");
+}
+
+inline void JfrVersionSystem::Node::commit() {
+  assert(version() != 0, "invariant");
+  // A commit consist of an atomic increment of the tip.
+  const Type commit_version = _system->inc_tip();
+  // Release this checkout.
+  set(0);
+  // Await release of checkouts for earlier versions.
+  _system->await(commit_version);
 }
 
 inline JfrVersionSystem::NodePtr
@@ -113,7 +138,7 @@ JfrVersionSystem::synchronize_with(JfrVersionSystem::Type version, JfrVersionSys
   return NULL;
 }
 
-inline void JfrVersionSystem::await(JfrVersionSystem::Type  version) {
+inline void JfrVersionSystem::await(JfrVersionSystem::Type version) {
   assert(version > 0, "invariant");
   static const int backoff_unit_ns = 10;
   int backoff_factor = 1;
@@ -127,64 +152,12 @@ inline void JfrVersionSystem::await(JfrVersionSystem::Type  version) {
   }
 }
 
-inline JfrVersionSystem::Handle JfrVersionSystem::get_handle() {
-  return Handle(this);
-}
-
-inline JfrVersionSystem::Handle JfrVersionSystem::checkout_handle() {
-  Handle handle(this);
-  handle.checkout();
-  return handle;
-}
-
-inline JfrVersionSystem::Handle::Handle(JfrVersionSystem* system) : _system(system), _node(system->acquire()) {}
-
-inline JfrVersionSystem::Handle::Handle() : _system(NULL), _node(NULL) {}
-
-inline JfrVersionSystem::Handle::~Handle() {
-  if (_node != NULL) {
-    _system->release(_node);
-  }
-}
-
-inline void JfrVersionSystem::Handle::checkout() {
-  assert(_node != NULL, "invariant");
-  _node->set(_system->tip());
-}
-
-inline JfrVersionSystem::Type JfrVersionSystem::Handle::increment() {
-  assert(_node != NULL, "invariant");
-  const Type version = _system->increment();
-  assert(version > _node->version(), "invariant");
-  return version;
-}
-
-inline void JfrVersionSystem::Handle::release() {
-  assert(_node != NULL, "invariant");
-  _system->release(_node);
-  _node = NULL;
-}
-
-inline void JfrVersionSystem::Handle::await(JfrVersionSystem::Type  version) {
-  _system->await(version);
-}
-
 #ifdef ASSERT
-inline bool JfrVersionSystem::is_registered(JfrVersionSystem::Type version) const {
-  NodePtr node = _head;
-  while (node != NULL) {
-    if (Atomic::load_acquire(&node->_version) == version) {
-      return true;
-    }
-    node = node->_next;
-  }
-  return false;
-}
-
-inline bool JfrVersionSystem::Handle::is_tracked() const {
-  assert(_node != NULL, "invariant");
-  const Type current_version = _node->version();
-  return current_version != 0 && _system->is_registered(current_version);
+inline void JfrVersionSystem::assert_state(const JfrVersionSystem::Node* node) const {
+  assert(node != NULL, "invariant");
+  assert(node->_live, "invariant");
+  assert(node->_version == 0, "invariant");
+  assert(node->_ref_counter.current() == 0, "invariant");
 }
 #endif // ASSERT