8256814: WeakProcessorPhases may be redundant

Remove WeakProcessorPhase, adding scoped enum categories to OopStorageSet.

Reviewed-by: stefank, tschatzl, rkennke

Author: Kim Barrett

src/hotspot/share/gc/g1/g1FullGCAdjustTask.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2017, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2017, 2021, Oracle and/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
@@ -31,7 +31,6 @@
 #include "gc/g1/g1StringDedup.hpp"
 #include "gc/g1/heapRegionManager.hpp"
 #include "gc/shared/parallelCleaning.hpp"
-#include "gc/shared/weakProcessorPhaseTimes.hpp"
 #include "gc/shared/weakProcessor.hpp"
 #include "utilities/ticks.hpp"
 

src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp

@@ -31,13 +31,15 @@
 #include "gc/g1/g1StringDedup.hpp"
 #include "gc/shared/gcTimer.hpp"
 #include "gc/shared/oopStorage.hpp"
+#include "gc/shared/oopStorageSet.hpp"
 #include "gc/shared/tlab_globals.hpp"
 #include "gc/shared/workerDataArray.inline.hpp"
 #include "memory/resourceArea.hpp"
 #include "logging/log.hpp"
 #include "logging/logStream.hpp"
 #include "runtime/timer.hpp"
 #include "runtime/os.hpp"
+#include "utilities/enumIterator.hpp"
 #include "utilities/macros.hpp"
 
 static const char* indent(uint level) {
@@ -64,13 +66,14 @@ G1GCPhaseTimes::G1GCPhaseTimes(STWGCTimer* gc_timer, uint max_gc_threads) :
   AOT_ONLY(_gc_par_phases[AOTCodeRoots] = new WorkerDataArray<double>("AOTCodeRoots", "AOT Root Scan (ms):", max_gc_threads);)
   _gc_par_phases[CMRefRoots] = new WorkerDataArray<double>("CMRefRoots", "CM RefProcessor Roots (ms):", max_gc_threads);
 
-  int index = G1GCPhaseTimes::StrongOopStorageSetRoots;
-  for (OopStorageSet::Iterator it = OopStorageSet::strong_iterator(); !it.is_end(); ++it, ++index) {
+  for (auto id : EnumRange<OopStorageSet::StrongId>()) {
+    GCParPhases phase = strong_oopstorage_phase(id);
     const char* phase_name_postfix = " Roots (ms):";
-    char* oop_storage_phase_name = NEW_C_HEAP_ARRAY(char, strlen(phase_name_postfix) + strlen(it->name()) + 1, mtGC);
-    strcpy(oop_storage_phase_name, it->name());
+    const char* storage_name = OopStorageSet::storage(id)->name();
+    char* oop_storage_phase_name = NEW_C_HEAP_ARRAY(char, strlen(phase_name_postfix) + strlen(storage_name) + 1, mtGC);
+    strcpy(oop_storage_phase_name, storage_name);
     strcat(oop_storage_phase_name, phase_name_postfix);
-    _gc_par_phases[index] = new WorkerDataArray<double>(it->name(), oop_storage_phase_name, max_gc_threads);
+    _gc_par_phases[phase] = new WorkerDataArray<double>(storage_name, oop_storage_phase_name, max_gc_threads);
   }
 
   _gc_par_phases[MergeER] = new WorkerDataArray<double>("MergeER", "Eager Reclaim (ms):", max_gc_threads);
@@ -487,7 +490,8 @@ double G1GCPhaseTimes::print_post_evacuate_collection_set() const {
 
   debug_time_for_reference("Reference Processing", _cur_ref_proc_time_ms);
   _ref_phase_times.print_all_references(2, false);
-  _weak_phase_times.log_print(2);
+  _weak_phase_times.log_total(2);
+  _weak_phase_times.log_subtotals(3);
 
   if (G1StringDedup::is_enabled()) {
     debug_time("String Deduplication", _cur_string_deduplication_time_ms);

src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2021, Oracle and/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
@@ -27,9 +27,10 @@
 
 #include "gc/shared/oopStorageSet.hpp"
 #include "gc/shared/referenceProcessorPhaseTimes.hpp"
-#include "gc/shared/weakProcessorPhaseTimes.hpp"
+#include "gc/shared/weakProcessorTimes.hpp"
 #include "logging/logLevel.hpp"
 #include "memory/allocation.hpp"
+#include "utilities/enumIterator.hpp"
 #include "utilities/macros.hpp"
 
 class LineBuffer;
@@ -51,10 +52,10 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
     CLDGRoots,
     AOT_ONLY(AOTCodeRoots COMMA)
     CMRefRoots,
-    // For every OopStorage there will be one element in the enum, starting with
-    // StrongOopStorageSetRoots.
+    // For every strong OopStorage there will be one element in this enum,
+    // starting with StrongOopStorageSetRoots.
     StrongOopStorageSetRoots,
-    MergeER = StrongOopStorageSetRoots + OopStorageSet::strong_count,
+    MergeER = StrongOopStorageSetRoots + EnumRange<OopStorageSet::StrongId>().size(),
     MergeRS,
     OptMergeRS,
     MergeLB,
@@ -84,6 +85,11 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
   static const GCParPhases ExtRootScanSubPhasesFirst = ThreadRoots;
   static const GCParPhases ExtRootScanSubPhasesLast = GCParPhases(MergeER - 1);
 
+  static constexpr GCParPhases strong_oopstorage_phase(OopStorageSet::StrongId id) {
+    size_t index = EnumRange<OopStorageSet::StrongId>().index(id);
+    return GCParPhases(StrongOopStorageSetRoots + index);
+  }
+
   enum GCMergeRSWorkTimes {
     MergeRSMergedSparse,
     MergeRSMergedFine,
@@ -187,7 +193,7 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
   double _cur_verify_after_time_ms;
 
   ReferenceProcessorPhaseTimes _ref_phase_times;
-  WeakProcessorPhaseTimes _weak_phase_times;
+  WeakProcessorTimes _weak_phase_times;
 
   double worker_time(GCParPhases phase, uint worker);
   void note_gc_end();
@@ -436,7 +442,7 @@ class G1GCPhaseTimes : public CHeapObj<mtGC> {
 
   ReferenceProcessorPhaseTimes* ref_phase_times() { return &_ref_phase_times; }
 
-  WeakProcessorPhaseTimes* weak_phase_times() { return &_weak_phase_times; }
+  WeakProcessorTimes* weak_phase_times() { return &_weak_phase_times; }
 };
 
 class G1EvacPhaseWithTrimTimeTracker : public StackObj {

src/hotspot/share/gc/g1/g1RootProcessor.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2021, Oracle and/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
@@ -44,6 +44,7 @@
 #include "gc/shared/referenceProcessor.hpp"
 #include "memory/allocation.inline.hpp"
 #include "runtime/mutex.hpp"
+#include "utilities/enumIterator.hpp"
 #include "utilities/macros.hpp"
 
 G1RootProcessor::G1RootProcessor(G1CollectedHeap* g1h, uint n_workers) :
@@ -195,10 +196,10 @@ void G1RootProcessor::process_vm_roots(G1RootClosures* closures,
   }
 #endif
 
-  for (int i = 0; i < _oop_storage_set_strong_par_state.par_state_count(); ++i) {
-    G1GCPhaseTimes::GCParPhases phase = G1GCPhaseTimes::GCParPhases(G1GCPhaseTimes::StrongOopStorageSetRoots + i);
+  for (auto id : EnumRange<OopStorageSet::StrongId>()) {
+    G1GCPhaseTimes::GCParPhases phase = G1GCPhaseTimes::strong_oopstorage_phase(id);
     G1GCParPhaseTimesTracker x(phase_times, phase, worker_id);
-    _oop_storage_set_strong_par_state.par_state(i)->oops_do(closures->strong_oops());
+    _oop_storage_set_strong_par_state.par_state(id)->oops_do(closures->strong_oops());
   }
 }
 

src/hotspot/share/gc/shared/oopStorageSet.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2021, Oracle and/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
@@ -29,62 +29,64 @@
 #include "utilities/globalDefinitions.hpp"
 #include "utilities/macros.hpp"
 
-// +1 for NULL singular entry.
-OopStorage* OopStorageSet::storages[all_count + 1] = {};
+OopStorage* OopStorageSet::_storages[all_count] = {};
 
 OopStorage* OopStorageSet::create_strong(const char* name) {
   static uint registered_strong = 0;
   assert(registered_strong < strong_count, "More registered strong storages than slots");
   OopStorage* storage = new OopStorage(name);
-  storages[strong_start + registered_strong++] = storage;
+  _storages[strong_start + registered_strong++] = storage;
   return storage;
 }
 
 OopStorage* OopStorageSet::create_weak(const char* name) {
   static uint registered_weak = 0;
   assert(registered_weak < weak_count, "More registered strong storages than slots");
   OopStorage* storage = new OopStorage(name);
-  storages[weak_start + registered_weak++] = storage;
+  _storages[weak_start + registered_weak++] = storage;
   return storage;
 }
 
 
 void OopStorageSet::fill_strong(OopStorage* to[strong_count]) {
   for (uint i = 0; i < OopStorageSet::strong_count; i++) {
-    to[i] = storage(strong_start + i);
+    to[i] = get_storage(strong_start + i);
   }
 }
 
 void OopStorageSet::fill_weak(OopStorage* to[weak_count]) {
   for (uint i = 0; i < OopStorageSet::weak_count; i++) {
-    to[i] = storage(weak_start + i);
+    to[i] = get_storage(weak_start + i);
   }
 }
 
 void OopStorageSet::fill_all(OopStorage* to[all_count]) {
   for (uint i = 0; i < OopStorageSet::all_count; i++) {
-    to[i] = storage(all_start + i);
+    to[i] = get_storage(all_start + i);
   }
 }
 
-#ifdef ASSERT
-
-void OopStorageSet::verify_initialized(uint index) {
-  assert(storages[index] != NULL, "oopstorage_init not yet called");
+OopStorage* OopStorageSet::get_storage(uint index) {
+  verify_initialized(index);
+  return _storages[index];
 }
 
-void OopStorageSet::Iterator::verify_nonsingular() const {
-  assert(_category != singular, "precondition");
+template<typename E>
+OopStorage* OopStorageSet::get_storage(E id) {
+  assert(EnumRange<E>().first() <= id, "invalid id");
+  assert(id <= EnumRange<E>().last(), "invalid id");
+  return get_storage(static_cast<uint>(id));
 }
 
-void OopStorageSet::Iterator::verify_category_match(const Iterator& other) const {
-  verify_nonsingular();
-  assert(_category == other._category, "precondition");
-}
+template OopStorage* OopStorageSet::get_storage(StrongId);
+template OopStorage* OopStorageSet::get_storage(WeakId);
+template OopStorage* OopStorageSet::get_storage(Id);
 
-void OopStorageSet::Iterator::verify_dereferenceable() const {
-  verify_nonsingular();
-  assert(!is_end(), "precondition");
+#ifdef ASSERT
+
+void OopStorageSet::verify_initialized(uint index) {
+  assert(index < ARRAY_SIZE(_storages), "invalid index");
+  assert(_storages[index] != NULL, "oopstorage_init not yet called");
 }
 
 #endif // ASSERT