8299614: Shenandoah: STW mark should keep nmethod/oops referenced from stack chunk alive

Reviewed-by: rkennke, zgu

src/hotspot/share/gc/shenandoah/mode/shenandoahIUMode.cpp

@@ -60,7 +60,6 @@ void ShenandoahIUMode::initialize_flags() const {
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahIUBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCASBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCloneBarrier);
-  SHENANDOAH_CHECK_FLAG_SET(ShenandoahNMethodBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahStackWatermarkBarrier);
 }
 

src/hotspot/share/gc/shenandoah/mode/shenandoahPassiveMode.cpp

@@ -50,7 +50,6 @@ void ShenandoahPassiveMode::initialize_flags() const {
   SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahIUBarrier);
   SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahCASBarrier);
   SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahCloneBarrier);
-  SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahNMethodBarrier);
   SHENANDOAH_ERGO_DISABLE_FLAG(ShenandoahStackWatermarkBarrier);
 
   // Final configuration checks

src/hotspot/share/gc/shenandoah/mode/shenandoahSATBMode.cpp

@@ -48,7 +48,6 @@ void ShenandoahSATBMode::initialize_flags() const {
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahSATBBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCASBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahCloneBarrier);
-  SHENANDOAH_CHECK_FLAG_SET(ShenandoahNMethodBarrier);
   SHENANDOAH_CHECK_FLAG_SET(ShenandoahStackWatermarkBarrier);
 }
 

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

@@ -45,7 +45,7 @@ ShenandoahBarrierSet::ShenandoahBarrierSet(ShenandoahHeap* heap) :
   BarrierSet(make_barrier_set_assembler<ShenandoahBarrierSetAssembler>(),
              make_barrier_set_c1<ShenandoahBarrierSetC1>(),
              make_barrier_set_c2<ShenandoahBarrierSetC2>(),
-             ShenandoahNMethodBarrier ? new ShenandoahBarrierSetNMethod(heap) : nullptr,
+             new ShenandoahBarrierSetNMethod(heap),
              new ShenandoahBarrierSetStackChunk(),
              BarrierSet::FakeRtti(BarrierSet::ShenandoahBarrierSet)),
   _heap(heap),

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

@@ -104,6 +104,20 @@ void ShenandoahParallelCodeHeapIterator::parallel_blobs_do(CodeBlobClosure* f) {
 ShenandoahNMethodTable* ShenandoahCodeRoots::_nmethod_table;
 int ShenandoahCodeRoots::_disarmed_value = 1;
 
+bool ShenandoahCodeRoots::use_nmethod_barriers_for_mark() {
+  // Continuations need nmethod barriers for scanning stack chunk nmethods.
+  if (Continuations::enabled()) return true;
+
+  // Concurrent class unloading needs nmethod barriers.
+  // When a nmethod is about to be executed, we need to make sure that all its
+  // metadata are marked. The alternative is to remark thread roots at final mark
+  // pause, which would cause latency issues.
+  if (ShenandoahHeap::heap()->unload_classes()) return true;
+
+  // Otherwise, we can go without nmethod barriers.
+  return false;
+}
+
 void ShenandoahCodeRoots::initialize() {
   _nmethod_table = new ShenandoahNMethodTable();
 }
@@ -118,8 +132,13 @@ void ShenandoahCodeRoots::unregister_nmethod(nmethod* nm) {
   _nmethod_table->unregister_nmethod(nm);
 }
 
-void ShenandoahCodeRoots::arm_nmethods() {
+void ShenandoahCodeRoots::arm_nmethods_for_mark() {
-  assert(BarrierSet::barrier_set()->barrier_set_nmethod() != nullptr, "Sanity");
+  if (use_nmethod_barriers_for_mark()) {
+    BarrierSet::barrier_set()->barrier_set_nmethod()->arm_all_nmethods();
+  }
+}
+
+void ShenandoahCodeRoots::arm_nmethods_for_evac() {
   BarrierSet::barrier_set()->barrier_set_nmethod()->arm_all_nmethods();
 }
 
@@ -163,7 +182,7 @@ public:
 };
 
 void ShenandoahCodeRoots::disarm_nmethods() {
-  if (ShenandoahNMethodBarrier) {
+  if (use_nmethod_barriers_for_mark()) {
     ShenandoahDisarmNMethodsTask task;
     ShenandoahHeap::heap()->workers()->run_task(&task);
   }

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

@@ -97,11 +97,14 @@ public:
   // Concurrent nmethod unloading support
   static void unlink(WorkerThreads* workers, bool unloading_occurred);
   static void purge();
-  static void arm_nmethods();
+  static void arm_nmethods_for_mark();
+  static void arm_nmethods_for_evac();
   static void disarm_nmethods();
   static int  disarmed_value()         { return _disarmed_value; }
   static int* disarmed_value_address() { return &_disarmed_value; }
 
+  static bool use_nmethod_barriers_for_mark();
+
 private:
   static ShenandoahNMethodTable* _nmethod_table;
   static int                     _disarmed_value;

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

@@ -545,12 +545,9 @@ void ShenandoahConcurrentGC::op_init_mark() {
 
   // Make above changes visible to worker threads
   OrderAccess::fence();
-  // Arm nmethods for concurrent marking. When a nmethod is about to be executed,
+
-  // we need to make sure that all its metadata are marked. alternative is to remark
+  // Arm nmethods for concurrent mark
-  // thread roots at final mark pause, but it can be potential latency killer.
+  ShenandoahCodeRoots::arm_nmethods_for_mark();
-  if (heap->unload_classes()) {
-    ShenandoahCodeRoots::arm_nmethods();
-  }
 
   ShenandoahStackWatermark::change_epoch_id();
   if (ShenandoahPacing) {
@@ -603,7 +600,7 @@ void ShenandoahConcurrentGC::op_final_mark() {
       }
 
       // Arm nmethods/stack for concurrent processing
-      ShenandoahCodeRoots::arm_nmethods();
+      ShenandoahCodeRoots::arm_nmethods_for_evac();
       ShenandoahStackWatermark::change_epoch_id();
 
       if (ShenandoahPacing) {

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

@@ -181,11 +181,9 @@ void ShenandoahDegenGC::op_degenerated() {
         assert(!heap->cancelled_gc(), "STW reference update can not OOM");
       }
 
-      if (ClassUnloading) {
+      // Disarm nmethods that armed in concurrent cycle.
-         // Disarm nmethods that armed in concurrent cycle.
+      // In above case, update roots should disarm them
-         // In above case, update roots should disarm them
+      ShenandoahCodeRoots::disarm_nmethods();
-         ShenandoahCodeRoots::disarm_nmethods();
-      }
 
       op_cleanup_complete();
       break;

src/hotspot/share/gc/shenandoah/shenandoahNMethod.inline.hpp

@@ -80,9 +80,7 @@ void ShenandoahNMethod::heal_nmethod_metadata(ShenandoahNMethod* nmethod_data) {
 
 void ShenandoahNMethod::disarm_nmethod(nmethod* nm) {
   BarrierSetNMethod* const bs = BarrierSet::barrier_set()->barrier_set_nmethod();
-  assert(bs != nullptr || !ShenandoahNMethodBarrier,
+  if (bs->is_armed(nm)) {
-        "Must have nmethod barrier for concurrent GC");
-  if (bs != nullptr && bs->is_armed(nm)) {
     bs->disarm(nm);
   }
 }

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

@@ -206,7 +206,7 @@ ShenandoahRootAdjuster::ShenandoahRootAdjuster(uint n_workers, ShenandoahPhaseTi
 void ShenandoahRootAdjuster::roots_do(uint worker_id, OopClosure* oops) {
   CodeBlobToOopClosure code_blob_cl(oops, CodeBlobToOopClosure::FixRelocations);
   ShenandoahCodeBlobAndDisarmClosure blobs_and_disarm_Cl(oops);
-  CodeBlobToOopClosure* adjust_code_closure = (ClassUnloading && ShenandoahNMethodBarrier) ?
+  CodeBlobToOopClosure* adjust_code_closure = ShenandoahCodeRoots::use_nmethod_barriers_for_mark() ?
                                               static_cast<CodeBlobToOopClosure*>(&blobs_and_disarm_Cl) :
                                               static_cast<CodeBlobToOopClosure*>(&code_blob_cl);
   CLDToOopClosure adjust_cld_closure(oops, ClassLoaderData::_claim_strong);

src/hotspot/share/gc/shenandoah/shenandoahRootProcessor.inline.hpp

@@ -172,7 +172,7 @@ template <typename IsAlive, typename KeepAlive>
 void ShenandoahRootUpdater::roots_do(uint worker_id, IsAlive* is_alive, KeepAlive* keep_alive) {
   CodeBlobToOopClosure update_blobs(keep_alive, CodeBlobToOopClosure::FixRelocations);
   ShenandoahCodeBlobAndDisarmClosure blobs_and_disarm_Cl(keep_alive);
-  CodeBlobToOopClosure* codes_cl = (ClassUnloading && ShenandoahNMethodBarrier) ?
+  CodeBlobToOopClosure* codes_cl = ShenandoahCodeRoots::use_nmethod_barriers_for_mark() ?
                                    static_cast<CodeBlobToOopClosure*>(&blobs_and_disarm_Cl) :
                                    static_cast<CodeBlobToOopClosure*>(&update_blobs);
 

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

@@ -89,8 +89,13 @@ ShenandoahSTWMark::ShenandoahSTWMark(bool full_gc) :
 }
 
 void ShenandoahSTWMark::mark() {
-  // Weak reference processing
   ShenandoahHeap* const heap = ShenandoahHeap::heap();
+
+  // Arm all nmethods. Even though this is STW mark, some marking code
+  // piggybacks on nmethod barriers for special instances.
+  ShenandoahCodeRoots::arm_nmethods_for_mark();
+
+  // Weak reference processing
   ShenandoahReferenceProcessor* rp = heap->ref_processor();
   rp->reset_thread_locals();
   rp->set_soft_reference_policy(heap->soft_ref_policy()->should_clear_all_soft_refs());
@@ -120,6 +125,9 @@ void ShenandoahSTWMark::mark() {
   heap->mark_complete_marking_context();
   end_mark();
 
+  // Mark is finished, can disarm the nmethods now.
+  ShenandoahCodeRoots::disarm_nmethods();
+
   assert(task_queues()->is_empty(), "Should be empty");
   TASKQUEUE_STATS_ONLY(task_queues()->print_taskqueue_stats());
   TASKQUEUE_STATS_ONLY(task_queues()->reset_taskqueue_stats());

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

@@ -352,9 +352,6 @@
   product(bool, ShenandoahLoadRefBarrier, true, DIAGNOSTIC,                 \
           "Turn on/off load-reference barriers in Shenandoah")              \
                                                                             \
-  product(bool, ShenandoahNMethodBarrier, true, DIAGNOSTIC,                 \
-          "Turn on/off NMethod entry barriers in Shenandoah")               \
-                                                                            \
   product(bool, ShenandoahStackWatermarkBarrier, true, DIAGNOSTIC,          \
           "Turn on/off stack watermark barriers in Shenandoah")             \
                                                                             \

test/hotspot/jtreg/gc/shenandoah/options/TestSelectiveBarrierFlags.java

@@ -54,7 +54,6 @@ public class TestSelectiveBarrierFlags {
                 new String[] { "ShenandoahSATBBarrier", "ShenandoahIUBarrier" },
                 new String[] { "ShenandoahCASBarrier" },
                 new String[] { "ShenandoahCloneBarrier" },
-                new String[] { "ShenandoahNMethodBarrier" },
                 new String[] { "ShenandoahStackWatermarkBarrier" }
         };
 

test/hotspot/jtreg/gc/shenandoah/options/TestWrongBarrierDisable.java

@@ -42,7 +42,6 @@ public class TestWrongBarrierDisable {
                 "ShenandoahSATBBarrier",
                 "ShenandoahCASBarrier",
                 "ShenandoahCloneBarrier",
-                "ShenandoahNMethodBarrier",
                 "ShenandoahStackWatermarkBarrier",
         };
         String[] iu = {
@@ -50,7 +49,6 @@ public class TestWrongBarrierDisable {
                 "ShenandoahIUBarrier",
                 "ShenandoahCASBarrier",
                 "ShenandoahCloneBarrier",
-                "ShenandoahNMethodBarrier",
                 "ShenandoahStackWatermarkBarrier",
         };