RFR: 8191565: Last-ditch Full GC should also move humongous objects

[Thomas Schatzl]

On Thu, 2 Mar 2023 13:48:10 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

> Hi All,
> 
> Please review this change to move humongous regions during the Last-Ditch full gc ( on `do_maximal_compaction`). This change will enable G1 to avoid encountering Out-Of-Memory errors that may occur due to the fragmentation of memory regions caused by the allocation of large memory blocks.
> 
> Here's how it works: At the end of `phase2_prepare_compaction`, G1 performs a serial compaction process for regular objects, which results in the heap being divided into two parts. The first part is a densely populated prefix that contains all the regular objects that have been moved. The second part consists of the remaining heap space, which may contain free regions, uncommitted regions, and regions that are not compacting. By moving/compacting the humongous objects in the second part of the heap closer to the dense prefix, G1 reduces the region fragmentation and avoids running into OOM errors.
> 
> We have enabled for G1 the Jtreg test that was previously used only for Shenandoah to test such workload. 
> 
> Testing: Tier 1-3

Changes requested by tschatzl (Reviewer).

src/hotspot/share/gc/g1/g1FullCollector.hpp line 87:

> 85:   G1FullGCCompactionPoint   _serial_compaction_point;
> 86:   G1FullGCCompactionPoint   _humongous_compaction_point;
> 87:   GrowableArray<HeapRegion*>* _humongous_compaction_regions;

This could be a `GrowableArrayCHeap<HeapRegion*, mtGC> _humongous_compaction_region` to avoid the necessity for explicit new/delete. Afaics there is no reassignment of that variable ever.

src/hotspot/share/gc/g1/g1FullCollector.hpp line 150:

> 148: 
> 149:   inline void add_humongous_region(HeapRegion* hr) { _humongous_compaction_regions->append(hr); }
> 150:   GrowableArray<HeapRegion*>* humongous_compaction_regions() { return _humongous_compaction_regions; }

Please move to the `.inline.hpp` file.

src/hotspot/share/gc/g1/g1FullGCCompactTask.cpp line 59:

> 57: 
> 58:   size_t size = obj->size();
> 59:   // copy object and reinit its mark

Suggestion:

  // Copy object and reinit its mark.

src/hotspot/share/gc/g1/g1FullGCCompactTask.cpp line 117:

> 115: }
> 116: 
> 117: void G1FullGCCompactTask::reset_humongous_metadata(HeapRegion* start_hr, uint num_regions, size_t word_size) {

Maybe this could somehow be refactored with `G1CollectedHeap::humongous_obj_allocate_initialize_regions`; probably not easily.

src/hotspot/share/gc/g1/g1FullGCCompactTask.cpp line 174:

> 172:   size_t word_size = obj->size();
> 173: 
> 174:   uint num_regions = (uint) G1CollectedHeap::humongous_obj_size_in_regions(word_size);

Suggestion:

  uint num_regions = (uint)G1CollectedHeap::humongous_obj_size_in_regions(word_size);

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 153:

> 151:   oop obj = cast_to_oop(hr->bottom());
> 152:   size_t obj_size = obj->size();
> 153:   int num_regions = (int) G1CollectedHeap::humongous_obj_size_in_regions(obj_size);

Please use signed int (or why not `size_t`?)

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 159:

> 157:   }
> 158: 
> 159:   // Find contiguous compaction target regions for the humongous object

Suggestion:

  // Find contiguous compaction target regions for the humongous object.

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 160:

> 158: 
> 159:   // Find contiguous compaction target regions for the humongous object
> 160:   Pair<uint, uint> range = find_contiguous_before(hr, num_regions);

(I'm suprised that MSVC does not complain here passing an int to the uint parameter)

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 165:

> 163: 
> 164:   if (range_begin == range_end) {
> 165:     // No contiguous compaction target regions found, so the object cannot be moved

Suggestion:

    // No contiguous compaction target regions found, so the object cannot be moved.

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 172:

> 170:   _collector->marker(0)->preserved_stack()->push_if_necessary(obj, obj->mark());
> 171: 
> 172:   HeapRegion* destn_hr = _compaction_regions->at(range_begin);

I would prefer `dest_hr` instead of `destn_hr` as abbreviation for "destination". It seems unusual.

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 176:

> 174:   assert(obj->is_forwarded(), "Object must be forwarded!");
> 175: 
> 176:   // Add the humongous object regions to the compaction point

Suggestion:

  // Add the humongous object regions to the compaction point.

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.cpp line 205:

> 203:     // Check if the current region and the previous region are contiguous.
> 204:     bool regions_are_contiguous = (_compaction_regions->at(range_end)->hrm_index() - _compaction_regions->at(range_end - 1)->hrm_index()) == 1;
> 205:     contiguous_region_count =  regions_are_contiguous ? contiguous_region_count + 1 : 1;

Suggestion:

    contiguous_region_count = regions_are_contiguous ? contiguous_region_count + 1 : 1;

src/hotspot/share/gc/g1/heapRegion.inline.hpp line 192:

> 190: }
> 191: 
> 192: inline void HeapRegion::reset_compacted_humongous_after_full_gc(HeapWord* new_top) {

This method could probably just call `reset_compacted_after_full_gc` as they are identical. (I am not suggestion to remove this method, although I'm not completely sure it's useful).

src/hotspot/share/utilities/growableArray.hpp line 261:

> 259: 
> 260:   // Remove all elements in the range [start - end). The order is preserved.
> 261:   void erase(int start, int end) {

I'd probably name this new method `remove_range` instead of using something completely unrelated.

-------------

PR: https://git.openjdk.org/jdk/pull/12830