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

walulyai · 2023-03-02T13:48:10Z

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

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

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

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 6f8dd514
Stefan Johansson (@kstefanj - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12830/head:pull/12830
$ git checkout pull/12830

Update a local copy of the PR:
$ git checkout pull/12830
$ git pull https://git.openjdk.org/jdk pull/12830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12830

View PR using the GUI difftool:
$ git pr show -t 12830

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12830.diff

bridgekeeper · 2023-03-02T13:49:35Z

👋 Welcome back iwalulya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-02T13:52:08Z

@walulyai The following labels will be automatically applied to this pull request:

hotspot
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-03-02T13:56:03Z

Webrevs

tschatzl · 2023-03-03T14:28:46Z

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

  G1FullGCMarker**          _markers;
  G1FullGCCompactionPoint** _compaction_points;
  OopQueueSet               _oop_queue_set;
  ObjArrayTaskQueueSet      _array_queue_set;
  PreservedMarksSet         _preserved_marks_set;
  G1FullGCCompactionPoint   _serial_compaction_point;
+  G1FullGCCompactionPoint   _humongous_compaction_point;
+  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.

tschatzl · 2023-03-03T14:30:10Z

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

+  inline void add_humongous_region(HeapRegion* hr) { _humongous_compaction_regions->append(hr); }
+  GrowableArray<HeapRegion*>* humongous_compaction_regions() { return _humongous_compaction_regions; }


Please move to the .inline.hpp file.

tschatzl · 2023-03-03T14:34:10Z

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

+  assert(obj->forwardee() != obj, "Object must have a new location");
+
+  size_t size = obj->size();
+  // copy object and reinit its mark


Suggested change

// copy object and reinit its mark

// Copy object and reinit its mark.

tschatzl · 2023-03-03T14:39:40Z

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

+  }
+}
+
+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.

tschatzl · 2023-03-03T14:43:40Z

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

+  oop obj = cast_to_oop(src_hr->bottom());
+  size_t word_size = obj->size();
+
+  uint num_regions = (uint) G1CollectedHeap::humongous_obj_size_in_regions(word_size);


Suggested change

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

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

tschatzl · 2023-03-03T14:48:18Z

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

+  // Preserve the mark for the humongous object as the region was initially not compacting.
+  _collector->marker(0)->preserved_stack()->push_if_necessary(obj, obj->mark());
+
+  HeapRegion* destn_hr = _compaction_regions->at(range_begin);


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

tschatzl · 2023-03-03T14:48:55Z

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

+  obj->forward_to(cast_to_oop(destn_hr->bottom()));
+  assert(obj->is_forwarded(), "Object must be forwarded!");
+
+  // Add the humongous object regions to the compaction point


Suggested change

// Add the humongous object regions to the compaction point

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

tschatzl · 2023-03-03T14:49:46Z

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

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


Suggested change

contiguous_region_count = regions_are_contiguous ? contiguous_region_count + 1 : 1;

contiguous_region_count = regions_are_contiguous ? contiguous_region_count + 1 : 1;

tschatzl · 2023-03-03T14:52:16Z

src/hotspot/share/gc/g1/heapRegion.inline.hpp


+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).

tschatzl · 2023-03-03T14:53:39Z

src/hotspot/share/utilities/growableArray.hpp

+  }
+
+  // Remove all elements in the range [start - end). The order is preserved.
+  void erase(int start, int end) {


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

tschatzl

Looks good.

openjdk · 2023-03-07T09:56:43Z

@walulyai This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: tschatzl, sjohanss

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 207 new commits pushed to the master branch:

f629152: 8304055: G1: Remove OldGCAllocRegion::release
b5108b4: 8303749: Serial: Use more strict card table API
7dbab81: 8304161: Add TypeKind.from to derive from TypeDescriptor.OfField
d4eb395: 8303684: Lift upcall sharing mechanism to AbstractLinker (mainline)
dfc7214: 8304283: Modernize the switch statements in jdk.internal.foreign
7277bb1: 8293324: ciField.hpp has two methods to return field's offset
b7945bc: 8303154: Investigate and improve instruction cache flushing during compilation
eefbaa2: 8283400: [macos] a11y : Screen magnifier does not reflect JRadioButton value change
42dd907: 8302906: AArch64: Add SVE backend support for vector unsigned comparison
2b81fae: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle
... and 197 more: https://git.openjdk.org/jdk/compare/3091744fff56ae08861f28b87c1de27738c4c62b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

albertnetymk · 2023-03-07T14:36:25Z

/label add hotspot-gc

openjdk · 2023-03-07T14:37:41Z

@albertnetymk
The hotspot-gc label was successfully added.

kstefanj

Overall it looks good, just a couple of small comments below.

Many thanks for taking on this change.

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

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

kstefanj

Looks good!

walulyai · 2023-03-16T14:14:37Z

Thanks @kstefanj and @tschatzl for the reviews!

/integrate

openjdk · 2023-03-16T14:15:50Z

Going to push as commit 96889bf.
Since your change was applied there have been 207 commits pushed to the master branch:

f629152: 8304055: G1: Remove OldGCAllocRegion::release
b5108b4: 8303749: Serial: Use more strict card table API
7dbab81: 8304161: Add TypeKind.from to derive from TypeDescriptor.OfField
d4eb395: 8303684: Lift upcall sharing mechanism to AbstractLinker (mainline)
dfc7214: 8304283: Modernize the switch statements in jdk.internal.foreign
7277bb1: 8293324: ciField.hpp has two methods to return field's offset
b7945bc: 8303154: Investigate and improve instruction cache flushing during compilation
eefbaa2: 8283400: [macos] a11y : Screen magnifier does not reflect JRadioButton value change
42dd907: 8302906: AArch64: Add SVE backend support for vector unsigned comparison
2b81fae: 8303022: "assert(allocates2(pc)) failed: not in CodeBuffer memory" When linking downcall handle
... and 197 more: https://git.openjdk.org/jdk/compare/3091744fff56ae08861f28b87c1de27738c4c62b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-16T14:16:08Z

@walulyai Pushed as commit 96889bf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

walulyai added 4 commits March 2, 2023 11:22

working prototype

db48b70

breakout large functioons

895576d

add test

0b9ca76

restructure

f4951d3

openjdk bot added the rfr Pull request is ready for review label Mar 2, 2023

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Mar 2, 2023

tschatzl suggested changes Mar 3, 2023

View reviewed changes

walulyai added 2 commits March 3, 2023 16:09

Thomas review

ebf3c1c

Refactor resetting humongous metadata

6f8dd51

tschatzl approved these changes Mar 7, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 7, 2023

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Mar 7, 2023

kstefanj suggested changes Mar 15, 2023

View reviewed changes

src/hotspot/share/gc/g1/g1CollectedHeap.hpp Outdated Show resolved Hide resolved

src/hotspot/share/gc/g1/g1FullGCCompactionPoint.hpp Outdated Show resolved Hide resolved

StefanJ review

ac6bb06

kstefanj approved these changes Mar 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 16, 2023

openjdk bot closed this Mar 16, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

walulyai commented Mar 2, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

mlbridge bot commented Mar 2, 2023 •

edited

Loading

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl Mar 3, 2023

tschatzl left a comment

openjdk bot commented Mar 7, 2023 •

edited

Loading

albertnetymk commented Mar 7, 2023

openjdk bot commented Mar 7, 2023

kstefanj left a comment

kstefanj left a comment

walulyai commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

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

	// copy object and reinit its mark
	// Copy object and reinit its mark.

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

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

	contiguous_region_count = regions_are_contiguous ? contiguous_region_count + 1 : 1;
	contiguous_region_count = regions_are_contiguous ? contiguous_region_count + 1 : 1;


		inline void HeapRegion::reset_compacted_humongous_after_full_gc(HeapWord* new_top) {

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

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

Conversation

walulyai commented Mar 2, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 2, 2023

openjdk bot commented Mar 2, 2023

mlbridge bot commented Mar 2, 2023 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tschatzl left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 7, 2023 • edited Loading

albertnetymk commented Mar 7, 2023

openjdk bot commented Mar 7, 2023

kstefanj left a comment

Choose a reason for hiding this comment

kstefanj left a comment

Choose a reason for hiding this comment

walulyai commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

openjdk bot commented Mar 16, 2023

walulyai commented Mar 2, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 2, 2023 •

edited

Loading

openjdk bot commented Mar 7, 2023 •

edited

Loading