8331557: Serial: Refactor SerialHeap::do_collection #19056

albertnetymk · 2024-05-02T10:48:12Z

It's probably easier to read the new code directly. The two classes in serialVMOperations serve as entrance points to invoke young/full GCs. Some previously hidden decisions are made more obvious, e.g. if a young-gc fails (or will probablly fail), fallback to full-gc.

Additionally, StatRecord is removed, because this kind of info-aggregation should be done outsite VM (by third-party tool).

Test: tier1-6

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-8331557: Serial: Refactor SerialHeap::do_collection (Enhancement - P4)

Reviewers

Guoxiong Li (@lgxbslgx - Reviewer) ⚠️ Review applies to b7c4f869
Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19056

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-02T10:48:48Z

👋 Welcome back ayang! 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 · 2024-05-02T10:48:54Z

@albertnetymk 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:

8331557: Serial: Refactor SerialHeap::do_collection

Reviewed-by: gli, iwalulya

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

openjdk · 2024-05-02T10:49:28Z

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

hotspot
serviceability

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.

albertnetymk · 2024-05-02T10:49:45Z

/cc hotspot-gc

openjdk · 2024-05-02T10:51:06Z

@albertnetymk
The hotspot-gc label was successfully added.

mlbridge · 2024-05-02T10:54:03Z

Webrevs

06: Full (8c415033)
05: Full (d16e7c6d)
04: Full (b7c4f869)
03: Full (3ef71f0a)
02: Full - Incremental (d8d5a13e)
01: Full (7375bfbf)
00: Full (2521919a)

openjdk · 2024-05-03T12:46:23Z

@albertnetymk Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

lgxbslgx

Nice refactor.

lgxbslgx · 2024-05-04T01:16:30Z

src/hotspot/share/gc/serial/serialHeap.cpp


-  gen->stat_record()->invocations++;
-  gen->stat_record()->accumulated_time.start();
+bool SerialHeap::do_young_gc(DefNewGeneration* young_gen, bool clear_soft_refs) {


The parameter DefNewGeneration* young_gen is not necessary. We can use the field SerialHeap::_young_gen directly.

lgxbslgx · 2024-05-04T02:26:40Z

src/hotspot/share/gc/serial/serialHeap.cpp

+  update_gc_stats(young_gen, false);

-  update_gc_stats(gen, full);


The method update_gc_stats is only used by young-gen to sample the promoted size. It is good to rename and simplify the related code. I filed https://bugs.openjdk.org/browse/JDK-8331684 to follow up.

lgxbslgx · 2024-05-04T02:34:15Z

src/hotspot/share/gc/serial/serialHeap.cpp

    Universe::verify("Before GC");
  }
+  gc_prologue(false);


The parameter full of the method SerialHeap::gc_prologue doesn't been used. Seems a leftover of JDK-8323993.

True; can probably fixed in a followup cleanup.

Filed https://bugs.openjdk.org/browse/JDK-8331723 to follow up.

lgxbslgx · 2024-05-04T02:39:38Z

src/hotspot/share/gc/serial/serialHeap.cpp

+  increment_total_collections(false);
+  const bool should_verify = total_collections() >= VerifyGCStartAt;
+  if (should_verify && VerifyBeforeGC) {
+    prepare_for_verify();
    Universe::verify("Before GC");


May the prefix of the verification log be better to specify the minor or full GC? Such as Before Minor GC here.

Other Universe::verify(" seems to not distinguish minor/major.

OK. If someone want to change all of them in the future, she/he can file another ticket to follow up.

lgxbslgx · 2024-05-04T02:57:43Z

src/hotspot/share/gc/serial/serialHeap.cpp

+void SerialHeap::collect_at_safepoint_no_gc_locker(bool full) {
+  assert(!GCLocker::is_active(), "precondition");
+  bool clear_soft_refs = must_clear_all_soft_refs();
+
+  if (!full) {
+    bool success = do_young_gc(_young_gen, clear_soft_refs);
+    if (success) {
+      return;
+    }
+    // Upgrade to Full-GC if young-gc fails
+  }
+  do_full_collection_no_gc_locker(clear_soft_refs);
+}


Please note the difference between the previous SerialHeap::do_collection and SerialHeap::collect_at_safepoint_no_gc_locker here. The previous SerialHeap::do_collection may invoke full GC according to the method SerialHeap::should_do_full_collection even the young GC succeeded. But SerialHeap::collect_at_safepoint_no_gc_locker only invokes full GC when the young GC failed (because of failed promotion). Such change makes the SerialHeap::should_do_full_collection has no user. If the behaviour of the SerialHeap::collect_at_safepoint_no_gc_locker is your intention, I think it is good to remove SerialHeap::should_do_full_collection.

Removed should_do_full_collection.

lgxbslgx

Looks good. One suggestion, but not necessary.

lgxbslgx · 2024-05-10T08:40:14Z

src/hotspot/share/gc/serial/serialHeap.cpp

+void SerialHeap::collect_at_safepoint_no_gc_locker(bool full) {
+  assert(!GCLocker::is_active(), "precondition");
+  bool clear_soft_refs = must_clear_all_soft_refs();
+
+  if (!full) {
+    bool success = do_young_gc(clear_soft_refs);
+    if (success) {
+      return;
+    }
+    // Upgrade to Full-GC if young-gc fails
+  }
+  do_full_collection_no_gc_locker(clear_soft_refs);
+}
+


Maybe the method do_young_gc can be renamed to do_young_collection or do_young_collection_no_gc_locker which is consistent with do_full_collection or do_full_collection_no_gc_locker.

walulyai · 2024-05-17T05:47:33Z

src/hotspot/share/gc/serial/serialHeap.cpp

-    assert(is_in_reserved(result), "result not in heap");
+    return result;
+  }
+


Would be nice to add a comment here to indicate that the previous collection could have shrunk the heap.

walulyai · 2024-05-17T06:00:07Z

src/hotspot/share/gc/serial/serialHeap.hpp

+  void do_full_collection_no_gc_locker(bool clear_all_soft_refs);
+
+  void collect_at_safepoint_no_gc_locker(bool full);


I am not very convinced by the naming of the methods with the "no_gc_locker" constraint. But I guess it is following same convention as "*at_safepoint" method naming.

How about calling them try_x and x for the public and private API, respectively, e.g. try_do_full_collection and do_full_collection?

I prefer that to the "no_gc_locker" emphasizing names

I can do that for try_collect_at_safepoint, but SerialHeap::do_full_collection is an API from CollectedHeap.

yeah, i only meant change those that are not "override" and have the "no_gclocker" postfix.

walulyai · 2024-05-17T06:01:23Z

src/hotspot/share/gc/serial/serialHeap.cpp

-                  false,               // is_tlab
-                  OldGen);             // last_generation
+void SerialHeap::do_full_collection_no_gc_locker(bool clear_all_soft_refs) {
+  IsSTWGCActiveMark gc_active_mark;


IsSTWGCActiveMark active_gc_mark;indo_young_collection_no_gc_locker, just choose one and be consistent with it

walulyai · 2024-05-17T06:02:31Z

src/hotspot/share/gc/serial/serialHeap.cpp

-    _young_gen->print_summary_info_on(&lsh);
-    _old_gen->print_summary_info_on(&lsh);
-  }
+ // Nothing


What is the Nothing supposed to convey here?

To emphasize that this empty method is intentional, inspired by ZCollectedHeap::print_tracing_info.

Then better to keep the verb in the comment // Does nothing

walulyai

Looks good! Thanks for the cleanup.

albertnetymk · 2024-05-17T09:07:47Z

Thanks for review.

/integrate

openjdk · 2024-05-17T09:09:04Z

Going to push as commit f1ce9b0.
Since your change was applied there has been 1 commit pushed to the master branch:

14198f5: 8329653: JLILaunchTest fails on AIX after JDK-8329131

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-17T09:09:09Z

@albertnetymk Pushed as commit f1ce9b0.

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

openjdk bot changed the title ~~8331557~~ 8331557: Serial: Refactor SerialHeap::do_collection May 2, 2024

openjdk bot added the rfr label May 2, 2024

openjdk bot added serviceability hotspot labels May 2, 2024

openjdk bot added the hotspot-gc label May 2, 2024

s1-do-collect

Loading
Loading status checks…

7375bfb

albertnetymk force-pushed the s1-do-collect branch from 2521919 to 7375bfb Compare May 3, 2024 12:45

lgxbslgx suggested changes May 4, 2024

View reviewed changes

albertnetymk added 4 commits May 6, 2024 11:05

Merge branch 'master' into s1-do-collect

b4b2bd3

review

Loading
Loading status checks…

d8d5a13

merge

Loading
Loading status checks…

3ef71f0

Merge branch 'master' into s1-do-collect

Loading
Loading status checks…

b7c4f86

lgxbslgx approved these changes May 10, 2024

View reviewed changes

albertnetymk added 2 commits May 10, 2024 10:52

review

05e5595

Merge branch 'master' into s1-do-collect

Loading
Loading status checks…

d16e7c6

albertnetymk mentioned this pull request May 14, 2024

8331723: Serial: Remove the unused parameter of the method SerialHeap::gc_prologue #19207

Closed

3 tasks

walulyai suggested changes May 17, 2024

View reviewed changes

openjdk bot added the ready label May 17, 2024

albertnetymk added 2 commits May 17, 2024 08:52

Merge branch 'master' into s1-do-collect

340919d

review

Loading
Loading status checks…

8c41503

walulyai approved these changes May 17, 2024

View reviewed changes

openjdk bot added the integrated label May 17, 2024

openjdk bot closed this May 17, 2024

openjdk bot removed ready rfr labels May 17, 2024

albertnetymk deleted the s1-do-collect branch May 17, 2024 09:09

albertnetymk mentioned this pull request Jul 8, 2024

8335902: Parallel: Refactor VM_ParallelGCFailedAllocation and VM_ParallelGCSystemGC #20077

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8331557: Serial: Refactor SerialHeap::do_collection #19056

8331557: Serial: Refactor SerialHeap::do_collection #19056

albertnetymk commented May 2, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 2, 2024

openjdk bot commented May 2, 2024 •

edited

Loading

openjdk bot commented May 2, 2024

albertnetymk commented May 2, 2024

openjdk bot commented May 2, 2024

mlbridge bot commented May 2, 2024 •

edited

Loading

openjdk bot commented May 3, 2024

lgxbslgx left a comment

lgxbslgx May 4, 2024

lgxbslgx May 4, 2024

lgxbslgx May 4, 2024

albertnetymk May 6, 2024

lgxbslgx May 6, 2024

lgxbslgx May 4, 2024

albertnetymk May 6, 2024

lgxbslgx May 6, 2024

lgxbslgx May 4, 2024

albertnetymk May 6, 2024

lgxbslgx left a comment

lgxbslgx May 10, 2024

walulyai May 17, 2024

walulyai May 17, 2024

albertnetymk May 17, 2024

walulyai May 17, 2024

albertnetymk May 17, 2024

walulyai May 17, 2024

walulyai May 17, 2024

walulyai May 17, 2024

albertnetymk May 17, 2024

walulyai May 17, 2024

walulyai left a comment

albertnetymk commented May 17, 2024

openjdk bot commented May 17, 2024

openjdk bot commented May 17, 2024

		update_gc_stats(young_gen, false);

		update_gc_stats(gen, full);

		void do_full_collection_no_gc_locker(bool clear_all_soft_refs);

		void collect_at_safepoint_no_gc_locker(bool full);

8331557: Serial: Refactor SerialHeap::do_collection #19056

8331557: Serial: Refactor SerialHeap::do_collection #19056

Conversation

albertnetymk commented May 2, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 2, 2024

openjdk bot commented May 2, 2024 • edited Loading

openjdk bot commented May 2, 2024

albertnetymk commented May 2, 2024

openjdk bot commented May 2, 2024

mlbridge bot commented May 2, 2024 • edited Loading

Webrevs

openjdk bot commented May 3, 2024

lgxbslgx left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgxbslgx left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walulyai left a comment

Choose a reason for hiding this comment

albertnetymk commented May 17, 2024

openjdk bot commented May 17, 2024

openjdk bot commented May 17, 2024

albertnetymk commented May 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 2, 2024 •

edited

Loading

mlbridge bot commented May 2, 2024 •

edited

Loading