JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc #3459

Hamlin-Li · 2021-04-13T10:07:17Z

in JDK-8262068, we have an enhancement for full gc which will skip compacting some regions with high survivor ratio. There might be some young regions among these skipped regions. These young regions are not filled with BOTs info as they're young.
But after full GC these young regions will become old regions, and the BOTs info is better to be filled for performance consideration.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Stefan Johansson (@kstefanj - Reviewer)

Contributors

Shoubing Ma <mashoubing1@huawei.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3459/head:pull/3459
$ git checkout pull/3459

Update a local copy of the PR:
$ git checkout pull/3459
$ git pull https://git.openjdk.java.net/jdk pull/3459/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3459

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3459.diff

bridgekeeper · 2021-04-13T10:07:55Z

👋 Welcome back mli! 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 · 2021-04-13T10:09:20Z

@Hamlin-Li The following label will be automatically applied to this pull request:

hotspot-gc

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

Hamlin-Li · 2021-04-13T10:09:33Z

/contributor add Shoubing Ma mashoubing1@huawei.com

openjdk · 2021-04-13T10:10:48Z

@Hamlin-Li Could not parse Shoubing Ma mashoubing1@huawei.com as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

mlbridge · 2021-04-13T10:14:08Z

Webrevs

Hamlin-Li · 2021-04-13T12:23:17Z

/contributor add Shoubing Ma mashoubing1@huawei.com

openjdk · 2021-04-13T12:23:35Z

@Hamlin-Li
Contributor Shoubing Ma <mashoubing1@huawei.com> successfully added.

kstefanj · 2021-04-13T12:28:27Z

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

+  HeapWord* limit = hr->top();
+  HeapWord* next_addr = hr->bottom();
+  HeapWord* threshold = hr->initialize_threshold();
+  HeapWord* pre_addr;
+
+  while (next_addr < limit) {
+    Prefetch::write(next_addr, PrefetchScanIntervalInBytes);
+    pre_addr = next_addr;
+    if (_bitmap->is_marked(next_addr)) {
+      oop obj = cast_to_oop(next_addr);
+      next_addr += obj->size();
+    } else {
+      next_addr = _bitmap->get_next_marked_addr(next_addr, limit);
+    }
+
+    if (next_addr > threshold) {
+      threshold = hr->cross_threshold(pre_addr, next_addr);
+    }
+  }
+  assert(next_addr == limit, "Should stop the scan at the limit.");
+}


I think this could be implemented using apply_to_marked_objects(). Let the closure keep track of the threshold for the region and the apply() function could look something like this:

size_t apply(oop object) { size_t size = object->size(); HeapWord* addr = cast_from_oop<HeapWord*>(object); HeapWord* next_addr = addr + size; if (next_addr > _threshold) { _threshold = hr->cross_threshold(addr, next_addr); } return size; }

I realize that we need to keep the previous address in the closure as well, for the case when we step over the threshold because of an unmarked object.

Hi Stefan, Thanks for the suggestion.
At first thought, I think it's a good way to reuse the traversal ability of HeapRegion::apply_to_marked_objects.

But when I try to implement it actually, seems to me it might make the code a little complicated.

For the first part, in the new G1UpdateBotClosure, we need to indroduce an _pre_addr as you suggested. this part is implemented as below, it's not that complicated, please check the following code snippet:

void G1FullGCPrepareTask::G1CalculatePointersClosure::update_bot(HeapRegion* hr) { G1UpdateBotClosure updateBot(hr); hr->apply_to_marked_objects(_bitmap, &updateBot); } G1FullGCPrepareTask::G1UpdateBotClosure::G1UpdateBotClosure(HeapRegion* hr) : _hr(hr), _pre_addr(hr->bottom()), _threshold(hr->initialize_threshold()) { } size_t G1FullGCPrepareTask::G1UpdateBotClosure::apply(oop object) { HeapWord* addr = cast_from_oop<HeapWord*>(object); size_t size = object->size(); HeapWord* next_addr = addr + size; if(addr > _threshold) { _threshold = _hr->cross_threshold(_pre_addr, addr); } if (next_addr > _threshold) { _threshold = _hr->cross_threshold(addr, next_addr); } _pre_addr = next_addr;+ return size; }

But, the above code does not consider the situation: one or several dead objects are at the end of the heap region, at this situation, we needs to update the bot outside of the G1UpdateBotClosure after hr->apply_to_marked_objects(...). I did not implement this part yet, but seems to me it makes the code a little complicated and not that readable, so would like to discuss with you first.

In summary, seems it does not make the code more readable, and will increase the complexity.
How do you think about it?

BTW, may I ask what's the format do you use the quote a snippet of code, seems my snippet of code lost all format info(e.g. indent).

Easiest is to mark the code and push the "Insert code" button (<>). You can also manually write ``` to start a code block.

I'll get back to you about the code above later.

Thanks Stefan, it works.

Looked a bit more at the code now and I agree that it didn't become as neat as I had hoped. Solving the problem you describe with dead objects at the end should be fairly easy in the closure-destructor I guess. Something like:

~G1UpdateBotClosure() { if (_hr->top() > _threshold) { _hr->cross_threshold(_pre_addr, _hr->top()); } }

And a few comments in the apply method will help readability:

size_t apply(oop object) { size_t size = object->size(); HeapWord* addr = cast_from_oop<HeapWord*>(object); HeapWord* next_addr = addr + size; // Threshold was crossed by dead object. if(addr > _threshold) { _threshold = _hr->cross_threshold(_pre_addr, addr); } // Threshold is crossed by this object. if (next_addr > _threshold) { _threshold = _hr->cross_threshold(addr, next_addr); } _pre_addr = next_addr; return size; }

But still it might be better to instead streamline the current approach:

Suggested change

HeapWord* limit = hr->top();

HeapWord* next_addr = hr->bottom();

HeapWord* threshold = hr->initialize_threshold();

HeapWord* pre_addr;

while (next_addr < limit) {

Prefetch::write(next_addr, PrefetchScanIntervalInBytes);

pre_addr = next_addr;

if (_bitmap->is_marked(next_addr)) {

oop obj = cast_to_oop(next_addr);

next_addr += obj->size();

} else {

next_addr = _bitmap->get_next_marked_addr(next_addr, limit);

}

if (next_addr > threshold) {

threshold = hr->cross_threshold(pre_addr, next_addr);

}

}

assert(next_addr == limit, "Should stop the scan at the limit.");

}

HeapWord* limit = hr->top();

HeapWord* next_addr = hr->bottom();

HeapWord* threshold = hr->initialize_threshold();

HeapWord* pre_addr;

while (next_addr < limit) {

pre_addr = next_addr;

next_addr = _bitmap->get_next_marked_addr(next_addr + 1, limit);

if (next_addr > threshold) {

threshold = hr->cross_threshold(pre_addr, next_addr);

}

}

assert(next_addr == limit, "Should stop the scan at the limit.");

}

Here I removed the Prefetch because we do not need to touch the heap. For this to be true I also removed the small optimization to use obj->size(), which is probably not an optimization if we don't need to do anything with the "object".

After doing this I agree that reusing apply_to_marked_objects() is probably not helping.

Hi Stefan, I think the refactoring you suggested is great, I just modified as you suggested and the pre-submit tests run successfully.

tschatzl

Minor nits.

tschatzl · 2021-04-15T08:00:18Z

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

+  HeapWord* limit = hr->top();
+  HeapWord* next_addr = hr->bottom();
+  HeapWord* threshold = hr->initialize_threshold();
+  HeapWord* pre_addr;


Minor nit: typically the "friend" of next is prev, not pre in traversals.

tschatzl · 2021-04-15T08:02:04Z

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

@@ -157,6 +166,22 @@ bool G1FullGCPrepareTask::G1CalculatePointersClosure::should_compact(HeapRegion*
  return live_words <= live_words_threshold;
 }

+void G1FullGCPrepareTask::G1CalculatePointersClosure::update_bot(HeapRegion* hr) {
+  HeapWord* limit = hr->top();


Maybe make this const.

tschatzl · 2021-04-15T08:07:13Z

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

@@ -67,6 +67,15 @@ bool G1FullGCPrepareTask::G1CalculatePointersClosure::do_heap_region(HeapRegion*
      // Force the high live ration region pinned,
      // as we need skip these regions in the later compact step.
      force_pinned = true;
+      if (hr->is_young()) {
+        // Old regions have BOTs info for performance consideration, but young regions


I'd replace this comment with something like this:

// G1 updates the BOT for old region contents incrementally, but young regions lack BOT // information for performance reasons. // Recreate BOT information of high live ratio young regions here to keep expected // performance during scanning their card tables in the collection pauses later.

Or something like this...

Thanks Thomas for refining the comments.

openjdk · 2021-04-15T11:42:18Z

@Hamlin-Li this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout fill-bots-in-full-gc
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

kstefanj

Looks good.

openjdk · 2021-04-15T15:33:08Z

@Hamlin-Li 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:

8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc

Co-authored-by: Shoubing Ma <mashoubing1@huawei.com>
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 19 new commits pushed to the master branch:

50f3da8: 8264480: Unreachable code in nmethod.cpp inside #ifdef DEBUG
e0151a6: 8264104: Eliminate unnecessary vector mask conversion during VectorUnbox for floating point VectorMask
64e2130: 8262108: SimpleDateFormat formatting broken for sq_MK Locale
3423f3e: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods
f6e54f2: 8258794: Support for CLDR version 39
e89fd15: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it
0b1b5c8: 8264373: javac hangs when annotation is declared with sealed public modifier
c7da64a: 8265302: ProblemList runtime/logging/RedefineClasses.java on linux-x64 -Xcomp
838c11f: 8265293: ProblemList java/foreign/TestDowncall.java on macosx-aarch64
325eecb: 8255273: jshell crashes with UnsupportedOperationException: Should not get here.
... and 9 more: https://git.openjdk.java.net/jdk/compare/9d669c912d3977027f321a5e6b7f045ca2ce9482...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.

Hamlin-Li · 2021-04-16T03:56:26Z

Thanks @kstefanj @tschatzl for review, I think I'm good to integrate this patch now, would you mind to help double confirm?

tschatzl

Ship it.

Hamlin-Li · 2021-04-16T08:06:13Z

/integrate

openjdk · 2021-04-16T08:07:26Z

@Hamlin-Li Since your change was applied there have been 20 commits pushed to the master branch:

fc89fe6: 8265119: G1: update_remset_before_rebuild mixes liveness in words with liveness in bytes
50f3da8: 8264480: Unreachable code in nmethod.cpp inside #ifdef DEBUG
e0151a6: 8264104: Eliminate unnecessary vector mask conversion during VectorUnbox for floating point VectorMask
64e2130: 8262108: SimpleDateFormat formatting broken for sq_MK Locale
3423f3e: 8265180: JvmtiCompiledMethodLoadEvent should include the stub section of nmethods
f6e54f2: 8258794: Support for CLDR version 39
e89fd15: 8261301: StringWriter.flush() is NOOP but documentation does not indicate it
0b1b5c8: 8264373: javac hangs when annotation is declared with sealed public modifier
c7da64a: 8265302: ProblemList runtime/logging/RedefineClasses.java on linux-x64 -Xcomp
838c11f: 8265293: ProblemList java/foreign/TestDowncall.java on macosx-aarch64
... and 10 more: https://git.openjdk.java.net/jdk/compare/9d669c912d3977027f321a5e6b7f045ca2ce9482...master

Your commit was automatically rebased without conflicts.

Pushed as commit b4ba74e.

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

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc

591e14b

openjdk bot added the rfr Pull request is ready for review label Apr 13, 2021

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Apr 13, 2021

kstefanj reviewed Apr 13, 2021

View reviewed changes

refine the code.

962e657

tschatzl suggested changes Apr 15, 2021

View reviewed changes

refine comments, names.

f3524ba

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 15, 2021

Merge branch 'master' into fill-bots-in-full-gc

ac24111

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 15, 2021

kstefanj approved these changes Apr 15, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 15, 2021

tschatzl approved these changes Apr 16, 2021

View reviewed changes

openjdk bot closed this Apr 16, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2021

Hamlin-Li deleted the fill-bots-in-full-gc branch April 16, 2021 08:10

jianglizhou mentioned this pull request May 31, 2021

JDK-8267562: G1: Missing BOT in Open Archive regions causes long pauses #4265

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc #3459

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc #3459

Hamlin-Li commented Apr 13, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

Hamlin-Li commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

mlbridge bot commented Apr 13, 2021 •

edited

Loading

Hamlin-Li commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

kstefanj Apr 13, 2021

kstefanj Apr 13, 2021

Hamlin-Li Apr 14, 2021 •

edited

Loading

Hamlin-Li Apr 14, 2021

kstefanj Apr 14, 2021

Hamlin-Li Apr 14, 2021

kstefanj Apr 14, 2021

Hamlin-Li Apr 14, 2021

tschatzl left a comment

tschatzl Apr 15, 2021

tschatzl Apr 15, 2021

tschatzl Apr 15, 2021

Hamlin-Li Apr 15, 2021

openjdk bot commented Apr 15, 2021

kstefanj left a comment

openjdk bot commented Apr 15, 2021 •

edited

Loading

Hamlin-Li commented Apr 16, 2021

tschatzl left a comment

Hamlin-Li commented Apr 16, 2021

openjdk bot commented Apr 16, 2021

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc #3459

JDK-8264987: G1: Fill BOTs for Survivor-turned-to-Old regions in full gc #3459

Conversation

Hamlin-Li commented Apr 13, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

Hamlin-Li commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

mlbridge bot commented Apr 13, 2021 • edited Loading

Webrevs

Hamlin-Li commented Apr 13, 2021

openjdk bot commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hamlin-Li Apr 14, 2021 • edited Loading

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

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

openjdk bot commented Apr 15, 2021

kstefanj left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 15, 2021 • edited Loading

Hamlin-Li commented Apr 16, 2021

tschatzl left a comment

Choose a reason for hiding this comment

Hamlin-Li commented Apr 16, 2021

openjdk bot commented Apr 16, 2021

Hamlin-Li commented Apr 13, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 13, 2021 •

edited

Loading

Hamlin-Li Apr 14, 2021 •

edited

Loading

openjdk bot commented Apr 15, 2021 •

edited

Loading