8051680: (ref) unnecessary process_soft_ref_reconsider

albertnetymk · albertnetymk · commit 86a20081aefb · 2021-07-12T07:53:20.000Z
Reviewed-by: kbarrett, tschatzl
diff --git a/src/hotspot/share/gc/shared/referenceProcessor.cpp b/src/hotspot/share/gc/shared/referenceProcessor.cpp
@@ -81,14 +81,6 @@ void ReferenceProcessor::enable_discovery(bool check_no_refs) {
   }
 #endif // ASSERT
 
-  // Someone could have modified the value of the static
-  // field in the j.l.r.SoftReference class that holds the
-  // soft reference timestamp clock using reflection or
-  // Unsafe between GCs. Unconditionally update the static
-  // field in ReferenceProcessor here so that we use the new
-  // value during reference discovery.
-
-  _soft_ref_timestamp_clock = java_lang_ref_SoftReference::clock();
   _discovering_refs = true;
 }
 
@@ -156,8 +148,6 @@ void ReferenceProcessor::update_soft_ref_master_clock() {
   // We need a monotonically non-decreasing time in ms but
   // os::javaTimeMillis() does not guarantee monotonicity.
   jlong now = os::javaTimeNanos() / NANOSECS_PER_MILLISEC;
-  jlong soft_ref_clock = java_lang_ref_SoftReference::clock();
-  assert(soft_ref_clock == _soft_ref_timestamp_clock, "soft ref clocks out of sync");
 
   NOT_PRODUCT(
   if (now < _soft_ref_timestamp_clock) {
@@ -201,26 +191,11 @@ ReferenceProcessorStats ReferenceProcessor::process_discovered_references(RefPro
   // Stop treating discovered references specially.
   disable_discovery();
 
-  // If discovery was concurrent, someone could have modified
-  // the value of the static field in the j.l.r.SoftReference
-  // class that holds the soft reference timestamp clock using
-  // reflection or Unsafe between when discovery was enabled and
-  // now. Unconditionally update the static field in ReferenceProcessor
-  // here so that we use the new value during processing of the
-  // discovered soft refs.
-
-  _soft_ref_timestamp_clock = java_lang_ref_SoftReference::clock();
-
   ReferenceProcessorStats stats(total_count(_discoveredSoftRefs),
                                 total_count(_discoveredWeakRefs),
                                 total_count(_discoveredFinalRefs),
                                 total_count(_discoveredPhantomRefs));
 
-  {
-    RefProcTotalPhaseTimesTracker tt(RefPhase1, &phase_times);
-    process_soft_ref_reconsider(proxy_task, phase_times);
-  }
-
   update_soft_ref_master_clock();
 
   {
@@ -329,37 +304,6 @@ inline void log_enqueued_ref(const DiscoveredListIterator& iter, const char* rea
   assert(oopDesc::is_oop(iter.obj()), "Adding a bad reference");
 }
 
-size_t ReferenceProcessor::process_soft_ref_reconsider_work(DiscoveredList&    refs_list,
-                                                            ReferencePolicy*   policy,
-                                                            BoolObjectClosure* is_alive,
-                                                            OopClosure*        keep_alive,
-                                                            VoidClosure*       complete_gc) {
-  assert(policy != NULL, "Must have a non-NULL policy");
-  DiscoveredListIterator iter(refs_list, keep_alive, is_alive);
-  // Decide which softly reachable refs should be kept alive.
-  while (iter.has_next()) {
-    iter.load_ptrs(DEBUG_ONLY(!discovery_is_atomic() /* allow_null_referent */));
-    bool referent_is_dead = (iter.referent() != NULL) && !iter.is_referent_alive();
-    if (referent_is_dead &&
-        !policy->should_clear_reference(iter.obj(), _soft_ref_timestamp_clock)) {
-      log_dropped_ref(iter, "by policy");
-      // Remove Reference object from list
-      iter.remove();
-      // keep the referent around
-      iter.make_referent_alive();
-      iter.move_to_next();
-    } else {
-      iter.next();
-    }
-  }
-  // Close the reachable set
-  complete_gc->do_void();
-
-  log_develop_trace(gc, ref)(" Dropped " SIZE_FORMAT " dead Refs out of " SIZE_FORMAT " discovered Refs by policy, from list " INTPTR_FORMAT,
-                             iter.removed(), iter.processed(), p2i(&refs_list));
-  return iter.removed();
-}
-
 size_t ReferenceProcessor::process_soft_weak_final_refs_work(DiscoveredList&    refs_list,
                                                              BoolObjectClosure* is_alive,
                                                              OopClosure*        keep_alive,
@@ -508,34 +452,6 @@ size_t ReferenceProcessor::total_reference_count(ReferenceType type) const {
 }
 
 
-
-class RefProcPhase1Task : public RefProcTask {
-public:
-  RefProcPhase1Task(ReferenceProcessor& ref_processor,
-                    ReferenceProcessorPhaseTimes* phase_times,
-                    ReferencePolicy* policy)
-    : RefProcTask(ref_processor,
-                  phase_times),
-      _policy(policy) { }
-
-  void rp_work(uint worker_id,
-               BoolObjectClosure* is_alive,
-               OopClosure* keep_alive,
-               VoidClosure* complete_gc) override {
-    ResourceMark rm;
-    RefProcSubPhasesWorkerTimeTracker tt(ReferenceProcessor::SoftRefSubPhase1, _phase_times, tracker_id(worker_id));
-    size_t const removed = _ref_processor.process_soft_ref_reconsider_work(_ref_processor._discoveredSoftRefs[worker_id],
-                                                                           _policy,
-                                                                           is_alive,
-                                                                           keep_alive,
-                                                                           complete_gc);
-    _phase_times->add_ref_cleared(REF_SOFT, removed);
-  }
-
-private:
-  ReferencePolicy* _policy;
-};
-
 class RefProcPhase2Task: public RefProcTask {
   void run_phase2(uint worker_id,
                   DiscoveredList list[],
@@ -791,38 +707,6 @@ void ReferenceProcessor::run_task(RefProcTask& task, RefProcProxyTask& proxy_tas
   }
 }
 
-void ReferenceProcessor::process_soft_ref_reconsider(RefProcProxyTask& proxy_task,
-                                                     ReferenceProcessorPhaseTimes& phase_times) {
-
-  size_t const num_soft_refs = total_count(_discoveredSoftRefs);
-  phase_times.set_ref_discovered(REF_SOFT, num_soft_refs);
-  phase_times.set_processing_is_mt(processing_is_mt());
-
-  if (num_soft_refs == 0) {
-    log_debug(gc, ref)("Skipped phase 1 of Reference Processing: no references");
-    return;
-  }
-
-  if (_current_soft_ref_policy == NULL) {
-    log_debug(gc, ref)("Skipped phase 1 of Reference Processing: no policy");
-    return;
-  }
-
-  RefProcMTDegreeAdjuster a(this, RefPhase1, num_soft_refs);
-
-  if (processing_is_mt()) {
-    RefProcBalanceQueuesTimeTracker tt(RefPhase1, &phase_times);
-    maybe_balance_queues(_discoveredSoftRefs);
-  }
-
-  RefProcPhaseTimeTracker tt(RefPhase1, &phase_times);
-
-  log_reflist("Phase 1 Soft before", _discoveredSoftRefs, _max_num_queues);
-  RefProcPhase1Task phase1(*this, &phase_times, _current_soft_ref_policy);
-  run_task(phase1, proxy_task, true);
-  log_reflist("Phase 1 Soft after", _discoveredSoftRefs, _max_num_queues);
-}
-
 void ReferenceProcessor::process_soft_weak_final_refs(RefProcProxyTask& proxy_task,
                                                       ReferenceProcessorPhaseTimes& phase_times) {
 
@@ -1305,8 +1189,8 @@ uint RefProcMTDegreeAdjuster::ergo_proc_thread_count(size_t ref_count,
 }
 
 bool RefProcMTDegreeAdjuster::use_max_threads(RefProcPhases phase) const {
-  // Even a small number of references in either of those cases could produce large amounts of work.
-  return (phase == ReferenceProcessor::RefPhase1 || phase == ReferenceProcessor::RefPhase3);
+  // Even a small number of references in this phase could produce large amounts of work.
+  return phase == ReferenceProcessor::RefPhase3;
 }
 
 RefProcMTDegreeAdjuster::RefProcMTDegreeAdjuster(ReferenceProcessor* rp,
diff --git a/src/hotspot/share/gc/shared/referenceProcessor.hpp b/src/hotspot/share/gc/shared/referenceProcessor.hpp
@@ -160,15 +160,13 @@ class DiscoveredListIterator {
 // straightforward manner in a general, non-generational, non-contiguous generation
 // (or heap) setting.
 class ReferenceProcessor : public ReferenceDiscoverer {
-  friend class RefProcPhase1Task;
   friend class RefProcPhase2Task;
   friend class RefProcPhase3Task;
   friend class RefProcPhase4Task;
 public:
   // Names of sub-phases of reference processing. Indicates the type of the reference
   // processed and the associated phase number at the end.
   enum RefProcSubPhases {
-    SoftRefSubPhase1,
     SoftRefSubPhase2,
     WeakRefSubPhase2,
     FinalRefSubPhase2,
@@ -179,7 +177,6 @@ class ReferenceProcessor : public ReferenceDiscoverer {
 
   // Main phases of reference processing.
   enum RefProcPhases {
-    RefPhase1,
     RefPhase2,
     RefPhase3,
     RefPhase4,
@@ -237,10 +234,6 @@ class ReferenceProcessor : public ReferenceDiscoverer {
 
   void run_task(RefProcTask& task, RefProcProxyTask& proxy_task, bool marks_oops_alive);
 
-  // Phase 1: Re-evaluate soft ref policy.
-  void process_soft_ref_reconsider(RefProcProxyTask& proxy_task,
-                                   ReferenceProcessorPhaseTimes& phase_times);
-
   // Phase 2: Drop Soft/Weak/Final references with a NULL or live referent, and clear
   // and enqueue non-Final references.
   void process_soft_weak_final_refs(RefProcProxyTask& proxy_task,
@@ -257,15 +250,6 @@ class ReferenceProcessor : public ReferenceDiscoverer {
   // Work methods used by the process_* methods. All methods return the number of
   // removed elements.
 
-  // (SoftReferences only) Traverse the list and remove any SoftReferences whose
-  // referents are not alive, but that should be kept alive for policy reasons.
-  // Keep alive the transitive closure of all such referents.
-  size_t process_soft_ref_reconsider_work(DiscoveredList&     refs_list,
-                                          ReferencePolicy*    policy,
-                                          BoolObjectClosure*  is_alive,
-                                          OopClosure*         keep_alive,
-                                          VoidClosure*        complete_gc);
-
   // Traverse the list and remove any Refs whose referents are alive,
   // or NULL if discovery is not atomic. Enqueue and clear the reference for
   // others if do_enqueue_and_clear is set.
diff --git a/src/hotspot/share/gc/shared/referenceProcessorPhaseTimes.cpp b/src/hotspot/share/gc/shared/referenceProcessorPhaseTimes.cpp
@@ -36,16 +36,15 @@
 #define ASSERT_REF_TYPE(ref_type) assert((ref_type) >= REF_SOFT && (ref_type) <= REF_PHANTOM, \
                                          "Invariant (%d)", (int)ref_type)
 
-#define ASSERT_PHASE(phase) assert((phase) >= ReferenceProcessor::RefPhase1 && \
+#define ASSERT_PHASE(phase) assert((phase) >= ReferenceProcessor::RefPhase2 && \
                                    (phase) < ReferenceProcessor::RefPhaseMax,  \
                                    "Invariant (%d)", (int)phase);
 
-#define ASSERT_SUB_PHASE(phase) assert((phase) >= ReferenceProcessor::SoftRefSubPhase1 && \
+#define ASSERT_SUB_PHASE(phase) assert((phase) >= ReferenceProcessor::SoftRefSubPhase2 && \
                                        (phase) < ReferenceProcessor::RefSubPhaseMax, \
                                        "Invariant (%d)", (int)phase);
 
 static const char* SubPhasesParWorkTitle[ReferenceProcessor::RefSubPhaseMax] = {
-       "SoftRef (ms):",
        "SoftRef (ms):",
        "WeakRef (ms):",
        "FinalRef (ms):",
@@ -56,7 +55,6 @@ static const char* SubPhasesParWorkTitle[ReferenceProcessor::RefSubPhaseMax] = {
 static const char* Phase2ParWorkTitle = "Total (ms):";
 
 static const char* SubPhasesSerWorkTitle[ReferenceProcessor::RefSubPhaseMax] = {
-       "SoftRef:",
        "SoftRef:",
        "WeakRef:",
        "FinalRef:",
@@ -69,7 +67,6 @@ static const char* Phase2SerWorkTitle = "Total:";
 static const char* Indents[6] = {"", "  ", "    ", "      ", "        ", "          "};
 
 static const char* PhaseNames[ReferenceProcessor::RefPhaseMax] = {
-       "Reconsider SoftReferences",
        "Notify Soft/WeakReferences",
        "Notify and keep alive finalizable",
        "Notify PhantomReferences"
@@ -275,7 +272,6 @@ void ReferenceProcessorPhaseTimes::print_all_references(uint base_indent, bool p
   }
 
   uint next_indent = base_indent + 1;
-  print_phase(ReferenceProcessor::RefPhase1, next_indent);
   print_phase(ReferenceProcessor::RefPhase2, next_indent);
   print_phase(ReferenceProcessor::RefPhase3, next_indent);
   print_phase(ReferenceProcessor::RefPhase4, next_indent);
@@ -329,9 +325,6 @@ void ReferenceProcessorPhaseTimes::print_phase(ReferenceProcessor::RefProcPhases
     }
 
     switch (phase) {
-      case ReferenceProcessor::RefPhase1:
-        print_sub_phase(&ls, ReferenceProcessor::SoftRefSubPhase1, indent + 1);
-        break;
       case ReferenceProcessor::RefPhase2:
         print_sub_phase(&ls, ReferenceProcessor::SoftRefSubPhase2, indent + 1);
         print_sub_phase(&ls, ReferenceProcessor::WeakRefSubPhase2, indent + 1);
diff --git a/test/hotspot/jtreg/gc/logging/TestPrintReferences.java b/test/hotspot/jtreg/gc/logging/TestPrintReferences.java
@@ -50,7 +50,6 @@ public class TestPrintReferences {
     static final String finalReference = "FinalReference";
     static final String phantomReference = "PhantomReference";
 
-    static final String phaseReconsiderSoftReferences = "Reconsider SoftReferences";
     static final String phaseNotifySoftWeakReferences = "Notify Soft/WeakReferences";
     static final String phaseNotifyKeepAliveFinalizer = "Notify and keep alive finalizable";
     static final String phaseNotifyPhantomReferences  = "Notify PhantomReferences";
@@ -137,7 +136,6 @@ private static void checkLogFormat(OutputAnalyzer output, boolean parallelRefPro
 
         final boolean p = parallelRefProcEnabled;
 
-        String phase1Regex = gcLogTimeRegex + phaseRegex(phaseReconsiderSoftReferences) + balanceRegex + subphaseRegex("SoftRef", p);
         String phase2Regex = gcLogTimeRegex + phaseRegex(phaseNotifySoftWeakReferences) +
                              balanceRegex +
                              subphaseRegex("SoftRef", p) +
@@ -148,7 +146,6 @@ private static void checkLogFormat(OutputAnalyzer output, boolean parallelRefPro
         String phase4Regex = gcLogTimeRegex + phaseRegex(phaseNotifyPhantomReferences) + balanceRegex + subphaseRegex("PhantomRef", p);
 
         output.shouldMatch(totalRegex +
-                           phase1Regex +
                            phase2Regex +
                            phase3Regex +
                            phase4Regex);
@@ -220,14 +217,14 @@ public static void checkLogValue(OutputAnalyzer out) {
     private static void checkTrimmedLogValue() {
         BigDecimal refProcTime = getTimeValue(referenceProcessing, 0);
 
-        BigDecimal sumOfSubPhasesTime = getTimeValue(phaseReconsiderSoftReferences, 2);
+        BigDecimal sumOfSubPhasesTime = BigDecimal.ZERO;
         sumOfSubPhasesTime = sumOfSubPhasesTime.add(getTimeValue(phaseNotifySoftWeakReferences, 2));
         sumOfSubPhasesTime = sumOfSubPhasesTime.add(getTimeValue(phaseNotifyKeepAliveFinalizer, 2));
         sumOfSubPhasesTime = sumOfSubPhasesTime.add(getTimeValue(phaseNotifyPhantomReferences, 2));
 
-        // If there are 4 phases, we should allow 0.2 tolerance.
-        final BigDecimal toleranceFor4SubPhases = BigDecimal.valueOf(0.2);
-        if (!greaterThanOrApproximatelyEqual(refProcTime, sumOfSubPhasesTime, toleranceFor4SubPhases)) {
+        // If there are 3 phases, we should allow 0.2 tolerance.
+        final BigDecimal toleranceFor3SubPhases = BigDecimal.valueOf(0.2);
+        if (!greaterThanOrApproximatelyEqual(refProcTime, sumOfSubPhasesTime, toleranceFor3SubPhases)) {
             throw new RuntimeException("Reference Processing time(" + refProcTime + "ms) is less than the sum("
                                        + sumOfSubPhasesTime + "ms) of each phases");
         }
