Merge

src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp

@@ -1283,11 +1283,16 @@ void ShenandoahHeap::object_iterate(ObjectClosure* cl) {
 
   Stack<oop,mtGC> oop_stack;
 
-  // First, we process GC roots according to current GC cycle. This populates the work stack with initial objects.
-  ShenandoahHeapIterationRootScanner rp;
   ObjectIterateScanRootClosure oops(&_aux_bit_map, &oop_stack);
 
-  rp.roots_do(&oops);
+  {
+    // First, we process GC roots according to current GC cycle.
+    // This populates the work stack with initial objects.
+    // It is important to relinquish the associated locks before diving
+    // into heap dumper.
+    ShenandoahHeapIterationRootScanner rp;
+    rp.roots_do(&oops);
+  }
 
   // Work through the oop stack to traverse heap.
   while (! oop_stack.is_empty()) {

src/hotspot/share/gc/shenandoah/shenandoahPacer.cpp

@@ -27,6 +27,7 @@
 #include "gc/shenandoah/shenandoahHeap.inline.hpp"
 #include "gc/shenandoah/shenandoahPacer.hpp"
 #include "runtime/atomic.hpp"
+#include "runtime/mutexLocker.hpp"
 
 /*
  * In normal concurrent cycle, we have to pace the application to let GC finish.
@@ -48,12 +49,8 @@
  * notion of progress is clear: we get reported the "used" size from the processed regions
  * and use the global heap-used as the baseline.
  *
- * The allocatable space when GC is running is "free" at the start of cycle, but the
+ * The allocatable space when GC is running is "free" at the start of phase, but the
  * accounted budget is based on "used". So, we need to adjust the tax knowing that.
- * Also, since we effectively count the used space three times (mark, evac, update-refs),
- * we need to multiply the tax by 3. Example: for 10 MB free and 90 MB used, GC would
- * come back with 3*90 MB budget, and thus for each 1 MB of allocation, we have to pay
- * 3*90 / 10 MBs. In the end, we would pay back the entire budget.
  */
 
 void ShenandoahPacer::setup_for_mark() {
@@ -66,7 +63,7 @@ void ShenandoahPacer::setup_for_mark() {
   size_t taxable = free - non_taxable;
 
   double tax = 1.0 * live / taxable; // base tax for available free space
-  tax *= 3;                          // mark is phase 1 of 3, claim 1/3 of free for it
+  tax *= 1;                          // mark can succeed with immediate garbage, claim all available space
   tax *= ShenandoahPacingSurcharge;  // additional surcharge to help unclutter heap
 
   restart_with(non_taxable, tax);
@@ -89,7 +86,7 @@ void ShenandoahPacer::setup_for_evac() {
   size_t taxable = free - non_taxable;
 
   double tax = 1.0 * used / taxable; // base tax for available free space
-  tax *= 2;                          // evac is phase 2 of 3, claim 1/2 of remaining free
+  tax *= 2;                          // evac is followed by update-refs, claim 1/2 of remaining free
   tax = MAX2<double>(1, tax);        // never allocate more than GC processes during the phase
   tax *= ShenandoahPacingSurcharge;  // additional surcharge to help unclutter heap
 
@@ -113,7 +110,7 @@ void ShenandoahPacer::setup_for_updaterefs() {
   size_t taxable = free - non_taxable;
 
   double tax = 1.0 * used / taxable; // base tax for available free space
-  tax *= 1;                          // update-refs is phase 3 of 3, claim the remaining free
+  tax *= 1;                          // update-refs is the last phase, claim the remaining free
   tax = MAX2<double>(1, tax);        // never allocate more than GC processes during the phase
   tax *= ShenandoahPacingSurcharge;  // additional surcharge to help unclutter heap
 
@@ -238,7 +235,7 @@ void ShenandoahPacer::pace_for_alloc(size_t words) {
   }
 
   // Threads that are attaching should not block at all: they are not
-  // fully initialized yet. Calling sleep() on them would be awkward.
+  // fully initialized yet. Blocking them would be awkward.
   // This is probably the path that allocates the thread oop itself.
   // Forcefully claim without waiting.
   if (JavaThread::current()->is_attaching_via_jni()) {
@@ -263,7 +260,7 @@ void ShenandoahPacer::pace_for_alloc(size_t words) {
     }
     cur = MAX2<size_t>(1, cur);
 
-    JavaThread::current()->sleep(cur);
+    wait(cur);
 
     double end = os::elapsedTime();
     total = (size_t)((end - start) * 1000);
@@ -288,6 +285,20 @@ void ShenandoahPacer::pace_for_alloc(size_t words) {
   }
 }
 
+void ShenandoahPacer::wait(size_t time_ms) {
+  // Perform timed wait. It works like like sleep(), except without modifying
+  // the thread interruptible status. MonitorLocker also checks for safepoints.
+  assert(time_ms > 0, "Should not call this with zero argument, as it would stall until notify");
+  assert(time_ms <= LONG_MAX, "Sanity");
+  MonitorLocker locker(_wait_monitor);
+  _wait_monitor->wait((long)time_ms);
+}
+
+void ShenandoahPacer::notify_waiters() {
+  MonitorLocker locker(_wait_monitor);
+  _wait_monitor->notify_all();
+}
+
 void ShenandoahPacer::print_on(outputStream* out) const {
   out->print_cr("ALLOCATION PACING:");
   out->cr();

src/hotspot/share/gc/shenandoah/shenandoahPacer.hpp

@@ -44,6 +44,7 @@ private:
   ShenandoahHeap* _heap;
   BinaryMagnitudeSeq _delays;
   TruncatedSeq* _progress_history;
+  Monitor* _wait_monitor;
 
   // Set once per phase
   volatile intptr_t _epoch;
@@ -63,6 +64,7 @@ public:
   ShenandoahPacer(ShenandoahHeap* heap) :
           _heap(heap),
           _progress_history(new TruncatedSeq(5)),
+          _wait_monitor(new Monitor(Mutex::leaf, "_wait_monitor", true, Monitor::_safepoint_check_always)),
           _epoch(0),
           _tax_rate(1),
           _budget(0),
@@ -97,6 +99,9 @@ private:
   void restart_with(size_t non_taxable_bytes, double tax_rate);
 
   size_t update_and_get_progress_history();
+
+  void wait(size_t time_ms);
+  void notify_waiters();
 };
 
 #endif // SHARE_GC_SHENANDOAH_SHENANDOAHPACER_HPP

src/java.base/share/classes/java/security/Provider.java

@@ -868,7 +868,7 @@ public abstract class Provider extends Properties {
     // For backward compatibility, the registration ordering of
     // SecureRandom (RNG) algorithms needs to be preserved for
     // "new SecureRandom()" calls when this provider is used
-    private transient Set<Service> prngServices;
+    private transient Set<String> prngAlgos;
 
     // Map<ServiceKey,Service>
     // used for services added via legacy methods, init on demand
@@ -1089,7 +1089,7 @@ public abstract class Provider extends Properties {
         legacyChanged = false;
         servicesChanged = false;
         serviceSet = null;
-        prngServices = null;
+        prngAlgos = null;
         super.clear();
         putId();
     }
@@ -1221,7 +1221,7 @@ public abstract class Provider extends Properties {
                 s.className = className;
 
                 if (type.equals("SecureRandom")) {
-                    updateSecureRandomEntries(true, s);
+                    updateSecureRandomEntries(true, s.algorithm);
                 }
             } else { // attribute
                 // e.g. put("MessageDigest.SHA-1 ImplementedIn", "Software");
@@ -1383,25 +1383,25 @@ public abstract class Provider extends Properties {
         synchronized (this) {
             putPropertyStrings(s);
             if (type.equals("SecureRandom")) {
-                updateSecureRandomEntries(true, s);
+                updateSecureRandomEntries(true, s.algorithm);
             }
         }
     }
 
-    private void updateSecureRandomEntries(boolean doAdd, Service s) {
+    // keep tracks of the registered secure random algos and store them in order
+    private void updateSecureRandomEntries(boolean doAdd, String s) {
         Objects.requireNonNull(s);
         if (doAdd) {
-            if (prngServices == null) {
+            if (prngAlgos == null) {
-                prngServices = new LinkedHashSet<Service>();
+                prngAlgos = new LinkedHashSet<String>();
             }
-            prngServices.add(s);
+            prngAlgos.add(s);
         } else {
-            prngServices.remove(s);
+            prngAlgos.remove(s);
         }
 
         if (debug != null) {
-            debug.println((doAdd? "Add":"Remove") + " SecureRandom algo " +
+            debug.println((doAdd? "Add":"Remove") + " SecureRandom algo " + s);
-                s.getAlgorithm());
         }
     }
 
@@ -1411,12 +1411,15 @@ public abstract class Provider extends Properties {
         checkInitialized();
 
         if (legacyChanged) {
-            prngServices = null;
+            prngAlgos = null;
             ensureLegacyParsed();
         }
 
-        if (prngServices != null && !prngServices.isEmpty()) {
+        if (prngAlgos != null && !prngAlgos.isEmpty()) {
-            return prngServices.iterator().next();
+            // IMPORTANT: use the Service obj returned by getService(...) call
+            // as providers may override putService(...)/getService(...) and
+            // return their own Service objects
+            return getService("SecureRandom", prngAlgos.iterator().next());
         }
 
         return null;
@@ -1516,7 +1519,7 @@ public abstract class Provider extends Properties {
         synchronized (this) {
             removePropertyStrings(s);
             if (type.equals("SecureRandom")) {
-                updateSecureRandomEntries(false, s);
+                updateSecureRandomEntries(false, s.algorithm);
             }
         }
     }

test/jdk/java/security/SecureRandom/DefaultAlgo.java

@@ -26,13 +26,12 @@ import java.security.Provider;
 import java.security.Security;
 import java.security.SecureRandom;
 import java.security.Provider.Service;
-import java.util.Objects;
 import java.util.Arrays;
 import sun.security.provider.SunEntries;
 
 /**
  * @test
- * @bug 8228613 8246613
+ * @bug 8228613 8246613 8248505
  * @summary Ensure that the default SecureRandom algo used is based
  *     on the registration ordering, and falls to next provider
  *     if none are found
@@ -40,6 +39,9 @@ import sun.security.provider.SunEntries;
  */
 public class DefaultAlgo {
 
+    private static final String SR_IMPLCLASS =
+            "sun.security.provider.SecureRandom";
+
     public static void main(String[] args) throws Exception {
         String[] algos = { "A", "B", "C" };
         test3rdParty(algos);
@@ -51,7 +53,8 @@ public class DefaultAlgo {
     private static void test3rdParty(String[] algos) {
         Provider[] provs = {
             new SampleLegacyProvider(algos),
-            new SampleServiceProvider(algos)
+            new SampleServiceProvider(algos),
+            new CustomProvider(algos)
         };
         for (Provider p : provs) {
             checkDefault(p, algos);
@@ -72,9 +75,10 @@ public class DefaultAlgo {
     }
 
     private static void checkDefault(Provider p, String ... algos) {
-        out.println(p.getName() + " with " + Arrays.toString(algos));
-        int pos = Security.insertProviderAt(p, 1);
         String pName = p.getName();
+        out.println(pName + " with " + Arrays.toString(algos));
+        int pos = Security.insertProviderAt(p, 1);
+
         boolean isLegacy = pName.equals("SampleLegacy");
         try {
             if (isLegacy) {
@@ -91,7 +95,13 @@ public class DefaultAlgo {
             out.println("=> Test Passed");
         } finally {
             if (pos != -1) {
-                Security.removeProvider(p.getName());
+                Security.removeProvider(pName);
+            }
+            if (isLegacy) {
+                // add back the removed algos
+                for (String s : algos) {
+                    p.put("SecureRandom." + s, SR_IMPLCLASS);
+                }
             }
         }
     }
@@ -100,7 +110,7 @@ public class DefaultAlgo {
         SampleLegacyProvider(String[] listOfSupportedRNGs) {
             super("SampleLegacy", "1.0", "test provider using legacy put");
             for (String s : listOfSupportedRNGs) {
-                put("SecureRandom." + s, "sun.security.provider.SecureRandom");
+                put("SecureRandom." + s, SR_IMPLCLASS);
             }
         }
     }
@@ -110,8 +120,37 @@ public class DefaultAlgo {
             super("SampleService", "1.0", "test provider using putService");
             for (String s : listOfSupportedRNGs) {
                 putService(new Provider.Service(this, "SecureRandom", s,
-                        "sun.security.provider.SecureRandom", null, null));
+                        SR_IMPLCLASS, null, null));
             }
         }
     }
+
+    // custom provider which overrides putService(...)/getService() and uses
+    // its own Service impl
+    private static class CustomProvider extends Provider {
+        private static class CustomService extends Provider.Service {
+            CustomService(Provider p, String type, String algo, String cName) {
+                super(p, type, algo, cName, null, null);
+            }
+        }
+
+        CustomProvider(String[] listOfSupportedRNGs) {
+            super("Custom", "1.0", "test provider overrides putService with " +
+                    " custom service with legacy registration");
+            for (String s : listOfSupportedRNGs) {
+                putService(new CustomService(this, "SecureRandom", s ,
+                        SR_IMPLCLASS));
+            }
+        }
+        @Override
+        protected void putService(Provider.Service s) {
+            // convert to legacy puts
+            put(s.getType() + "." + s.getAlgorithm(), s.getClassName());
+            put(s.getType() + ":" + s.getAlgorithm(), s);
+        }
+        @Override
+        public Provider.Service getService(String type, String algo) {
+            return (Provider.Service) get(type + ":" + algo);
+        }
+    }
 }