Albert review

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

@@ -2637,7 +2637,7 @@ bool G1CollectedHeap::has_uncommittable_regions() {
 
 void G1CollectedHeap::uncommit_heap_if_necessary() {
   if (has_uncommittable_regions()) {
-    G1UncommitRegionTask::activate();
+    G1UncommitRegionTask::run();
   }
 }
 

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

@@ -60,7 +60,9 @@ class G1CommittedRegionMap : public CHeapObj<mtGC> {
 
   uint max_length() const;
 
-  // Helpers to mark and do accounting for the bitmaps.
+  // Helpers to mark and do accounting for the bitmaps. Depending on when called
+  // these helpers require to own different locks. See guarantee_mt_safty_* for
+  // details.
   void active_set_range(uint start, uint end);
   void active_clear_range(uint start, uint end);
   void inactive_set_range(uint start, uint end);

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

@@ -52,7 +52,8 @@ G1RegionToSpaceMapper::G1RegionToSpaceMapper(ReservedSpace rs,
 
 // Used to manually signal a mapper to handle a set of regions as committed.
 // Setting the 'zero_filled' parameter to false signals the mapper that the
-// regions have not been cleared by the OS.
+// regions have not been cleared by the OS and that they need to be clear
+// explicitly.
 void G1RegionToSpaceMapper::signal_mapping_changed(uint start_idx, size_t num_regions) {
   fire_on_commit(start_idx, num_regions, false);
 }

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

@@ -33,7 +33,7 @@ G1UncommitRegionTask* G1UncommitRegionTask::_instance = NULL;
 
 G1UncommitRegionTask::G1UncommitRegionTask() :
     G1ServiceTask("G1 Uncommit Region Task"),
-    _state(TaskState::inactive),
+    _active(false),
     _summary_duration(),
     _summary_region_count(0) { }
 
@@ -43,7 +43,7 @@ void G1UncommitRegionTask::initialize() {
 
   // Register the task with the service thread. This will automatically
   // schedule the task so  we change the state to active.
-  _instance->set_state(TaskState::active);
+  _instance->set_active(true);
   G1CollectedHeap::heap()->service_thread()->register_task(_instance);
 }
 
@@ -54,26 +54,26 @@ G1UncommitRegionTask* G1UncommitRegionTask::instance() {
   return _instance;
 }
 
-void G1UncommitRegionTask::activate() {
+void G1UncommitRegionTask::run() {
   assert_at_safepoint_on_vm_thread();
 
   G1UncommitRegionTask* uncommit_task = instance();
   if (!uncommit_task->is_active()) {
     // Change state to active and schedule with no delay.
-    uncommit_task->set_state(TaskState::active);
+    uncommit_task->set_active(true);
     uncommit_task->schedule(0);
-    // Notify service thread to avoid unnecesarry waiting.
+    // Notify service thread to avoid unnecessary waiting.
     G1CollectedHeap::heap()->service_thread()->notify();
   }
 }
 
 bool G1UncommitRegionTask::is_active() {
-  return _state == TaskState::active;
+  return _active;
 }
 
-void G1UncommitRegionTask::set_state(TaskState state) {
-  assert(_state != state, "Must do a state change");
-  _state = state;
+void G1UncommitRegionTask::set_active(bool state) {
+  assert(_active != state, "Must do a state change");
+  _active = state;
 }
 
 void G1UncommitRegionTask::report_execution(Tickspan time, uint regions) {
@@ -101,9 +101,11 @@ void G1UncommitRegionTask::clear_summary() {
 }
 
 void G1UncommitRegionTask::execute() {
-  assert(_state == TaskState::active, "Must be active");
+  assert(_active, "Must be active");
 
-  // Each execution is limited to uncommit at most 256M worth of regions.
+  // Each execution is limited to uncommit at most 256M worth of regions. This
+  // limit is small enough to ensure that the duration of each invocation is
+  // short, while still making reasonable progress.
   static const uint region_limit = (uint) (256 * M / G1HeapRegionSize);
 
   // Prevent from running during a GC pause.
@@ -125,7 +127,8 @@ void G1UncommitRegionTask::execute() {
     // other tasks to run without waiting for a full uncommit cycle.
     schedule(0);
   } else {
-    set_state(TaskState::inactive);
+    // Nothing more to do, change state and report a summary.
+    set_active(false);
     report_summary();
     clear_summary();
   }

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

@@ -34,13 +34,12 @@ class G1UncommitRegionTask : public G1ServiceTask {
   static void initialize();
   static G1UncommitRegionTask* instance();
 
-  // The state is not guarded by any lock because the places
-  // where it is updated can never run concurrently. The state
-  // is set to active only from a safepoint and it is set to
-  // inactive while running on the service thread joined with
-  // the suspendible thread set.
-  enum class TaskState { active, inactive };
-  TaskState _state;
+  // The _active state is not guarded by a lock since the places
+  // where it is updated can never run in parallel. The state is
+  // set to active only from a safepoint and it is set to false
+  // while running on the service thread joined with the suspendible
+  // thread set.
+  bool _active;
 
   // Members to keep a summary of the current concurrent uncommit
   // work. Used for printing when no more work is available.
@@ -49,15 +48,15 @@ class G1UncommitRegionTask : public G1ServiceTask {
 
   G1UncommitRegionTask();
   bool is_active();
-  void set_state(TaskState state);
+  void set_active(bool state);
 
   void report_execution(Tickspan time, uint regions);
   void report_summary();
   void clear_summary();
 
 public:
-  static void activate();
+  static void run();
   virtual void execute();
 };
 
-#endif
+#endif

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

@@ -167,7 +167,6 @@ HeapRegion* HeapRegionManager::new_heap_region(uint hrm_index) {
 }
 
 void HeapRegionManager::expand(uint start, uint num_regions, WorkGang* pretouch_gang) {
-  guarantee(num_regions > 0, "No point in calling this for zero regions");
   commit_regions(start, num_regions, pretouch_gang);
   for (uint i = start; i < start + num_regions; i++) {
     HeapRegion* hr = _regions.get_by_index(i);
@@ -259,16 +258,16 @@ void HeapRegionManager::reactivate_regions(uint start, uint num_regions) {
   initialize_regions(start, num_regions);
 }
 
-void HeapRegionManager::deactivate_regions(uint start, size_t num_regions) {
-  guarantee(num_regions >= 1, "Need to specify at least one region to uncommit, tried to uncommit zero regions at %u", start);
-  guarantee(length() >= num_regions, "pre-condition");
+void HeapRegionManager::deactivate_regions(uint start, uint num_regions) {
+  assert(num_regions > 0, "Need to specify at least one region to uncommit, tried to uncommit zero regions at %u", start);
+  assert(length() >= num_regions, "pre-condition");
 
   G1HRPrinter* printer = G1CollectedHeap::heap()->hr_printer();
   bool printer_active = printer->is_active();
 
   // Reset NUMA index to and print if active.
-  uint end = (uint) (start + num_regions);
-  for (uint i = start; i < start + num_regions; i++) {
+  uint end = start + num_regions;
+  for (uint i = start; i < end; i++) {
     HeapRegion* hr = at(i);
     hr->set_node_index(G1NUMA::UnknownNodeIndex);
     if (printer_active) {
@@ -280,10 +279,14 @@ void HeapRegionManager::deactivate_regions(uint start, size_t num_regions) {
 }
 
 void HeapRegionManager::clear_auxiliary_data_structures(uint start, uint num_regions) {
+  // Signal marking bitmaps to clear the given regions.
   _prev_bitmap_mapper->signal_mapping_changed(start, num_regions);
   _next_bitmap_mapper->signal_mapping_changed(start, num_regions);
+  // Signal G1BlockOffsetTable to clear the given regions.
   _bot_mapper->signal_mapping_changed(start, num_regions);
+  // Signal G1CardTable to clear the given regions.
   _cardtable_mapper->signal_mapping_changed(start, num_regions);
+  // Signal G1CardCounts to clear the given regions.
   _card_counts_mapper->signal_mapping_changed(start, num_regions);
 }
 
@@ -310,14 +313,15 @@ bool HeapRegionManager::has_inactive_regions() const {
 }
 
 uint HeapRegionManager::uncommit_inactive_regions(uint limit) {
-  guarantee(limit >= 1, "Need to specify at least one region to uncommit");
+  assert(limit > 0, "Need to specify at least one region to uncommit");
 
   uint uncommitted = 0;
   uint offset = 0;
   do {
     MutexLocker uc(Uncommit_lock, Mutex::_no_safepoint_check_flag);
     HeapRegionRange range = _committed_map.next_inactive_range(offset);
-    // No more regions available for uncommit
+    // No more regions available for uncommit. Return the number of regions
+    // already uncommitted or 0 if there were no longer any inactive regions.
     if (range.length() == 0) {
       return uncommitted;
     }
@@ -661,7 +665,7 @@ void HeapRegionManager::shrink_at(uint index, size_t num_regions) {
   }
 #endif
   // Mark regions as inactive making them ready for uncommit.
-  deactivate_regions(index, num_regions);
+  deactivate_regions(index, (uint) num_regions);
 }
 
 uint HeapRegionManager::find_empty_from_idx_reverse(uint start_idx, uint* res_idx) const {

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

@@ -78,7 +78,8 @@ class HeapRegionManager: public CHeapObj<mtGC> {
   G1RegionToSpaceMapper* _cardtable_mapper;
   G1RegionToSpaceMapper* _card_counts_mapper;
 
-  // Map to keep track of which regions are in use.
+  // Keeps track of the currently committed regions in the heap. The committed regions
+  // can either be active (ready for use) or inactive (ready for uncommit).
   G1CommittedRegionMap _committed_map;
 
   // Internal only. The highest heap region +1 we allocated a HeapRegion instance for.
@@ -119,7 +120,7 @@ class HeapRegionManager: public CHeapObj<mtGC> {
 
   // Clear the auxiliary data structures by notifying them that the mapping has
   // changed. The structures that needs to be cleared will than clear. This is
-  // used to allow reuse regions scheduled for uncommit without uncommiting and
+  // used to allow reuse regions scheduled for uncommit without uncommitting and
   // then committing them.
   void clear_auxiliary_data_structures(uint start, uint num_regions);
 
@@ -129,9 +130,9 @@ class HeapRegionManager: public CHeapObj<mtGC> {
   G1RegionToSpaceMapper* _next_bitmap_mapper;
   FreeRegionList _free_list;
 
-  void expand(uint index, uint num_regions = 1, WorkGang* pretouch_gang = NULL);
-  void activate_regions(uint index, uint num_regions = 1);
-  void deactivate_regions(uint start, size_t num_regions);
+  void expand(uint index, uint num_regions, WorkGang* pretouch_gang = NULL);
+  void activate_regions(uint index, uint num_regions);
+  void deactivate_regions(uint start, uint num_regions);
   void reactivate_regions(uint start, uint num_regions);
   void uncommit_regions(uint start, uint num_regions);