8343704: Bad GC parallelism with processing Cleaner queues

Reviewed-by: bchristi, vklang, ogillespie, kdnilsen

src/java.base/share/classes/jdk/internal/ref/CleanerImpl.java

@@ -48,9 +48,9 @@ public final class CleanerImpl implements Runnable {
     private static Function<Cleaner, CleanerImpl> cleanerImplAccess = null;
 
     /**
-     * Heads of a CleanableList for each reference type.
+     * Currently active PhantomCleanable-s.
      */
-    final PhantomCleanable<?> phantomCleanableList;
+    final CleanableList activeList;
 
     // The ReferenceQueue of pending cleaning actions
     final ReferenceQueue<Object> queue;
@@ -82,7 +82,7 @@ static CleanerImpl getCleanerImpl(Cleaner cleaner) {
      */
     public CleanerImpl() {
         queue = new ReferenceQueue<>();
-        phantomCleanableList = new PhantomCleanableRef();
+        activeList = new CleanableList();
     }
 
     /**
@@ -129,7 +129,7 @@ public void run() {
         InnocuousThread mlThread = (t instanceof InnocuousThread)
                 ? (InnocuousThread) t
                 : null;
-        while (!phantomCleanableList.isListEmpty()) {
+        while (!activeList.isEmpty()) {
             if (mlThread != null) {
                 // Clear the thread locals
                 mlThread.eraseThreadLocals();
@@ -165,14 +165,6 @@ public PhantomCleanableRef(Object obj, Cleaner cleaner, Runnable action) {
             this.action = action;
         }
 
-        /**
-         * Constructor used only for root of phantom cleanable list.
-         */
-        PhantomCleanableRef() {
-            super();
-            this.action = null;
-        }
-
         @Override
         protected void performCleanup() {
             action.run();
@@ -231,4 +223,137 @@ protected void performCleanup() {
             // no action
         }
     }
+
+    /**
+     * A specialized implementation that tracks phantom cleanables.
+     */
+    static final class CleanableList {
+        /**
+         * Capacity for a single node in the list.
+         * This balances memory overheads vs locality vs GC walking costs.
+         */
+        static final int NODE_CAPACITY = 4096;
+
+        /**
+         * Head node. This is the only node where PhantomCleanables are
+         * added to or removed from. This is the only node with variable size,
+         * all other nodes linked from the head are always at full capacity.
+         */
+        private Node head;
+
+        /**
+         * Cached node instance to provide better behavior near NODE_CAPACITY
+         * threshold: if list size flips around NODE_CAPACITY, it would reuse
+         * the cached node instead of wasting and re-allocating a new node all
+         * the time.
+         */
+        private Node cache;
+
+        public CleanableList() {
+            reset();
+        }
+
+        /**
+         * Testing support: reset list to initial state.
+         */
+        synchronized void reset() {
+            this.head = new Node();
+        }
+
+        /**
+         * Returns true if cleanable list is empty.
+         *
+         * @return true if the list is empty
+         */
+        public synchronized boolean isEmpty() {
+            // Head node size is zero only when the entire list is empty.
+            return head.size == 0;
+        }
+
+        /**
+         * Insert this PhantomCleanable in the list.
+         */
+        public synchronized void insert(PhantomCleanable<?> phc) {
+            if (head.size == NODE_CAPACITY) {
+                // Head node is full, insert new one.
+                // If possible, pick a pre-allocated node from cache.
+                Node newHead;
+                if (cache != null) {
+                    newHead = cache;
+                    cache = null;
+                } else {
+                    newHead = new Node();
+                }
+                newHead.next = head;
+                head = newHead;
+            }
+            assert head.size < NODE_CAPACITY;
+
+            // Put the incoming object in head node and record indexes.
+            final int lastIndex = head.size;
+            phc.node = head;
+            phc.index = lastIndex;
+            head.arr[lastIndex] = phc;
+            head.size++;
+        }
+
+        /**
+         * Remove this PhantomCleanable from the list.
+         *
+         * @return true if Cleanable was removed or false if not because
+         * it had already been removed before
+         */
+        public synchronized boolean remove(PhantomCleanable<?> phc) {
+            if (phc.node == null) {
+                // Not in the list.
+                return false;
+            }
+            assert phc.node.arr[phc.index] == phc;
+
+            // Replace with another element from the head node, as long
+            // as it is not the same element. This keeps all non-head
+            // nodes at full capacity.
+            final int lastIndex = head.size - 1;
+            assert lastIndex >= 0;
+            if (head != phc.node || (phc.index != lastIndex)) {
+                PhantomCleanable<?> mover = head.arr[lastIndex];
+                mover.node = phc.node;
+                mover.index = phc.index;
+                phc.node.arr[phc.index] = mover;
+            }
+
+            // Now we can unlink the removed element.
+            phc.node = null;
+
+            // Remove the last element from the head node.
+            head.arr[lastIndex] = null;
+            head.size--;
+
+            // If head node becomes empty after this, and there are
+            // nodes that follow it, replace the head node with another
+            // full one. If needed, stash the now free node in cache.
+            if (head.size == 0 && head.next != null) {
+                Node newHead = head.next;
+                if (cache == null) {
+                    cache = head;
+                    cache.next = null;
+                }
+                head = newHead;
+            }
+
+            return true;
+        }
+
+        /**
+         * Segment node.
+         */
+        static class Node {
+            // Array of tracked cleanables, and the amount of elements in it.
+            final PhantomCleanable<?>[] arr = new PhantomCleanable<?>[NODE_CAPACITY];
+            int size;
+
+            // Linked list structure.
+            Node next;
+        }
+    }
 }

src/java.base/share/classes/jdk/internal/ref/PhantomCleanable.java

@@ -44,14 +44,21 @@ public abstract class PhantomCleanable<T> extends PhantomReference<T>
         implements Cleaner.Cleanable {
 
     /**
-     * Links to previous and next in a doubly-linked list.
+     * The list of PhantomCleanable; synchronizes insert and remove.
      */
-    PhantomCleanable<?> prev = this, next = this;
+    private final CleanerImpl.CleanableList list;
 
     /**
-     * The list of PhantomCleanable; synchronizes insert and remove.
+     * Node for this PhantomCleanable in the list.
+     * Synchronized by the same lock as the list itself.
      */
-    private final PhantomCleanable<?> list;
+    CleanerImpl.CleanableList.Node node;
+
+    /**
+     * Index of this PhantomCleanable in the list node.
+     * Synchronized by the same lock as the list itself.
+     */
+    int index;
 
     /**
      * Constructs new {@code PhantomCleanable} with
@@ -62,73 +69,29 @@ public abstract class PhantomCleanable<T> extends PhantomReference<T>
      * @param referent the referent to track
      * @param cleaner  the {@code Cleaner} to register with
      */
+    @SuppressWarnings("this-escape")
     public PhantomCleanable(T referent, Cleaner cleaner) {
         super(Objects.requireNonNull(referent), CleanerImpl.getCleanerImpl(cleaner).queue);
-        this.list = CleanerImpl.getCleanerImpl(cleaner).phantomCleanableList;
-        insert();
+        index = -1;
+        list = CleanerImpl.getCleanerImpl(cleaner).activeList;
+        list.insert(this);
+
+        // Check that list insertion populated the backlinks.
+        assert node != null;
+        assert index >= 0;
 
         // Ensure referent and cleaner remain accessible
         Reference.reachabilityFence(referent);
         Reference.reachabilityFence(cleaner);
     }
 
-    /**
-     * Construct a new root of the list; not inserted.
-     */
-    PhantomCleanable() {
-        super(null, null);
-        this.list = this;
-    }
-
-    /**
-     * Insert this PhantomCleanable after the list head.
-     */
-    private void insert() {
-        synchronized (list) {
-            prev = list;
-            next = list.next;
-            next.prev = this;
-            list.next = this;
-        }
-    }
-
-    /**
-     * Remove this PhantomCleanable from the list.
-     *
-     * @return true if Cleanable was removed or false if not because
-     * it had already been removed before
-     */
-    private boolean remove() {
-        synchronized (list) {
-            if (next != this) {
-                next.prev = prev;
-                prev.next = next;
-                prev = this;
-                next = this;
-                return true;
-            }
-            return false;
-        }
-    }
-
-    /**
-     * Returns true if the list's next reference refers to itself.
-     *
-     * @return true if the list is empty
-     */
-    boolean isListEmpty() {
-        synchronized (list) {
-            return list == list.next;
-        }
-    }
-
     /**
      * Unregister this PhantomCleanable and invoke {@link #performCleanup()},
      * ensuring at-most-once semantics.
      */
     @Override
     public final void clean() {
-        if (remove()) {
+        if (list.remove(this)) {
             super.clear();
             performCleanup();
         }
@@ -140,7 +103,7 @@ public final void clean() {
      */
     @Override
     public void clear() {
-        if (remove()) {
+        if (list.remove(this)) {
             super.clear();
         }
     }