8319048: Monitor deflation unlink phase prolongs time to safepoint

Reviewed-by: phh
Backport-of: efc392259c64986bbbe880259e95b09058b9076a

src/hotspot/share/runtime/globals.hpp

@@ -723,6 +723,10 @@ const intx ObjectAlignmentInBytes = 8;
           "at one time (minimum is 1024).")                      \
           range(1024, max_jint)                                             \
                                                                             \
+  product(intx, MonitorUnlinkBatch, 500, DIAGNOSTIC,                        \
+          "The maximum number of monitors to unlink in one batch. ")        \
+          range(1, max_jint)                                                \
+                                                                            \
   product(intx, MonitorUsedDeflationThreshold, 90, DIAGNOSTIC,              \
           "Percentage of used monitors before triggering deflation (0 is "  \
           "off). The check is performed on GuaranteedSafepointInterval, "   \

src/hotspot/share/runtime/synchronizer.cpp

@@ -80,44 +80,68 @@ size_t MonitorList::max() const {
   return Atomic::load(&_max);
 }
 
-// Walk the in-use list and unlink (at most MonitorDeflationMax) deflated
-// ObjectMonitors. Returns the number of unlinked ObjectMonitors.
+// Walk the in-use list and unlink deflated ObjectMonitors.
+// Returns the number of unlinked ObjectMonitors.
 size_t MonitorList::unlink_deflated(Thread* current, LogStream* ls,
                                     elapsedTimer* timer_p,
+                                    size_t deflated_count,
                                     GrowableArray<ObjectMonitor*>* unlinked_list) {
   size_t unlinked_count = 0;
-  ObjectMonitor* prev = NULL;
-  ObjectMonitor* head = Atomic::load_acquire(&_head);
-  ObjectMonitor* m = head;
-  // The in-use list head can be NULL during the final audit.
-  while (m != NULL) {
+  ObjectMonitor* prev = nullptr;
+  ObjectMonitor* m = Atomic::load_acquire(&_head);
+
+  // The in-use list head can be null during the final audit.
+  while (m != nullptr) {
     if (m->is_being_async_deflated()) {
-      // Find next live ObjectMonitor.
+      // Find next live ObjectMonitor. Batch up the unlinkable monitors, so we can
+      // modify the list once per batch. The batch starts at "m".
+      size_t unlinked_batch = 0;
       ObjectMonitor* next = m;
+      // Look for at most MonitorUnlinkBatch monitors, or the number of
+      // deflated and not unlinked monitors, whatever comes first.
+      assert(deflated_count >= unlinked_count, "Sanity: underflow");
+      size_t unlinked_batch_limit = MIN2<size_t>(deflated_count - unlinked_count, MonitorUnlinkBatch);
       do {
         ObjectMonitor* next_next = next->next_om();
-        unlinked_count++;
+        unlinked_batch++;
         unlinked_list->append(next);
         next = next_next;
-        if (unlinked_count >= (size_t)MonitorDeflationMax) {
-          // Reached the max so bail out on the gathering loop.
+        if (unlinked_batch >= unlinked_batch_limit) {
+          // Reached the max batch, so bail out of the gathering loop.
+          break;
+        }
+        if (prev == nullptr && Atomic::load(&_head) != m) {
+          // Current batch used to be at head, but it is not at head anymore.
+          // Bail out and figure out where we currently are. This avoids long
+          // walks searching for new prev during unlink under heavy list inserts.
           break;
         }
-      } while (next != NULL && next->is_being_async_deflated());
-      if (prev == NULL) {
-        ObjectMonitor* prev_head = Atomic::cmpxchg(&_head, head, next);
-        if (prev_head != head) {
-          // Find new prev ObjectMonitor that just got inserted.
+      } while (next != nullptr && next->is_being_async_deflated());
+
+      // Unlink the found batch.
+      if (prev == nullptr) {
+        // The current batch is the first batch, so there is a chance that it starts at head.
+        // Optimistically assume no inserts happened, and try to unlink the entire batch from the head.
+        ObjectMonitor* prev_head = Atomic::cmpxchg(&_head, m, next);
+        if (prev_head != m) {
+          // Something must have updated the head. Figure out the actual prev for this batch.
           for (ObjectMonitor* n = prev_head; n != m; n = n->next_om()) {
             prev = n;
           }
+          assert(prev != nullptr, "Should have found the prev for the current batch");
           prev->set_next_om(next);
         }
       } else {
+        // The current batch is preceded by another batch. This guarantees the current batch
+        // does not start at head. Unlink the entire current batch without updating the head.
+        assert(Atomic::load(&_head) != m, "Sanity");
         prev->set_next_om(next);
       }
-      if (unlinked_count >= (size_t)MonitorDeflationMax) {
-        // Reached the max so bail out on the searching loop.
+
+      unlinked_count += unlinked_batch;
+      if (unlinked_count >= deflated_count) {
+        // Reached the max so bail out of the searching loop.
+        // There should be no more deflated monitors left.
         break;
       }
       m = next;
@@ -133,6 +157,20 @@ size_t MonitorList::unlink_deflated(Thread* current, LogStream* ls,
                                             ls, timer_p);
     }
   }
+
+#ifdef ASSERT
+  // Invariant: the code above should unlink all deflated monitors.
+  // The code that runs after this unlinking does not expect deflated monitors.
+  // Notably, attempting to deflate the already deflated monitor would break.
+  {
+    ObjectMonitor* m = Atomic::load_acquire(&_head);
+    while (m != nullptr) {
+      assert(!m->is_being_async_deflated(), "All deflated monitors should be unlinked");
+      m = m->next_om();
+    }
+  }
+#endif
+
   Atomic::sub(&_count, unlinked_count);
   return unlinked_count;
 }
@@ -1520,6 +1558,7 @@ size_t ObjectSynchronizer::deflate_idle_monitors() {
     ResourceMark rm;
     GrowableArray<ObjectMonitor*> delete_list((int)deflated_count);
     size_t unlinked_count = _in_use_list.unlink_deflated(current, ls, &timer,
+                                                         deflated_count,
                                                          &delete_list);
     if (current->is_Java_thread()) {
       if (ls != NULL) {

src/hotspot/share/runtime/synchronizer.hpp

@@ -46,6 +46,7 @@ class MonitorList {
 public:
   void add(ObjectMonitor* monitor);
   size_t unlink_deflated(Thread* current, LogStream* ls, elapsedTimer* timer_p,
+                         size_t deflated_count,
                          GrowableArray<ObjectMonitor*>* unlinked_list);
   size_t count() const;
   size_t max() const;

test/hotspot/jtreg/runtime/Monitor/MonitorUnlinkBatchTest.java

@@ -0,0 +1,178 @@
+/*
+ * Copyright Amazon.com Inc. 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.
+ */
+
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.process.ProcessTools;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/*
+ * @test id=defaults
+ * @bug 8319048
+ * @summary Test the MonitorUnlinkBatch options
+ * @library /test/lib
+ * @run driver MonitorUnlinkBatchTest defaults
+ */
+
+/*
+ * @test id=legal
+ * @library /test/lib
+ * @run driver MonitorUnlinkBatchTest legal
+ */
+
+/*
+ * @test id=illegal
+ * @library /test/lib
+ * @run driver MonitorUnlinkBatchTest illegal
+ */
+
+/*
+ * @test id=aggressive
+ * @library /test/lib
+ * @run driver MonitorUnlinkBatchTest aggressive
+ */
+
+/*
+ * @test id=lazy
+ * @library /test/lib
+ * @run driver MonitorUnlinkBatchTest lazy
+ */
+
+
+public class MonitorUnlinkBatchTest {
+
+    public static class Test {
+        // Inflate a lot of monitors, so that threshold heuristics definitely fires
+        private static final int MONITORS = 10_000;
+
+        // Use a handful of threads to inflate the monitors, to eat the cost of
+        // wait(1) calls. This can be larger than available parallelism, since threads
+        // would be time-waiting.
+        private static final int THREADS = 16;
+
+        private static Thread[] threads;
+        private static Object[] monitors;
+
+        public static void main(String... args) throws Exception {
+            monitors = new Object[MONITORS];
+            threads = new Thread[THREADS];
+
+            for (int t = 0; t < THREADS; t++) {
+                int monStart = t * MONITORS / THREADS;
+                int monEnd = (t + 1) * MONITORS / THREADS;
+                threads[t] = new Thread(() -> {
+                    for (int m = monStart; m < monEnd; m++) {
+                        Object o = new Object();
+                        synchronized (o) {
+                            try {
+                                o.wait(1);
+                            } catch (InterruptedException e) {
+                            }
+                        }
+                        monitors[m] = o;
+                    }
+                });
+                threads[t].start();
+            }
+
+            for (Thread t : threads) {
+                t.join();
+            }
+
+            try {
+                Thread.sleep(10_000);
+            } catch (InterruptedException ie) {
+            }
+        }
+    }
+
+    public static void main(String[] args) throws Exception {
+        if (args.length < 1) {
+            throw new IllegalArgumentException("Expect the test label");
+        }
+
+        String test = args[0];
+        switch (test) {
+            case "defaults":
+                test("");
+                break;
+
+            case "legal":
+                // Legal, even if not useful settings
+                test("",
+                     "-XX:MonitorDeflationMax=100000",
+                     "-XX:MonitorUnlinkBatch=100001"
+                     );
+                break;
+
+            case "illegal":
+                // Quick tests that should fail on JVM flags verification.
+                test("outside the allowed range",
+                     "-XX:MonitorUnlinkBatch=-1"
+                );
+                test("outside the allowed range",
+                     "-XX:MonitorUnlinkBatch=0"
+                );
+                break;
+
+            case "aggressive":
+                // The smallest batch possible.
+                test("",
+                     "-XX:MonitorUnlinkBatch=1"
+                );
+                break;
+
+            case "lazy":
+                // The largest batch possible.
+                test("",
+                     "-XX:MonitorDeflationMax=1000000",
+                     "-XX:MonitorUnlinkBatch=1000000"
+                );
+                break;
+
+            default:
+                throw new IllegalArgumentException("Unknown test: " + test);
+        }
+    }
+
+    public static void test(String msg, String... args) throws Exception {
+        List<String> opts = new ArrayList<>();
+        opts.add("-Xmx128M");
+        opts.add("-XX:+UnlockDiagnosticVMOptions");
+        opts.add("-XX:GuaranteedAsyncDeflationInterval=100");
+        opts.addAll(Arrays.asList(args));
+        opts.add("MonitorUnlinkBatchTest$Test");
+
+        ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(opts);
+        OutputAnalyzer oa = new OutputAnalyzer(pb.start());
+        if (msg.isEmpty()) {
+            oa.shouldHaveExitValue(0);
+        } else {
+            oa.shouldNotHaveExitValue(0);
+            oa.shouldContain(msg);
+        }
+    }
+
+}