8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs #19306

albertnetymk · 2024-05-20T09:05:09Z

Simple heuristic update around how promoted-bytes are tracked. The real change is in TenuredGeneration::update_promote_stats, which uses used-old-gen before/after gc to derived promoted-bytes.

The metric is imprecise in the cases of promotion-failure, because those should be rare. Also, after full-gc, used-old-gen an be decreased; in this case, we'd set promoted bytes as the min value (zero), the best-effort estimate.

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-8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs (Enhancement - P4)

Reviewers

Ivan Walulya (@walulyai - Reviewer)
Thomas Schatzl (@tschatzl - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19306

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-20T09:05:56Z

👋 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-20T09:06:03Z

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

8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs

Reviewed-by: iwalulya, tschatzl

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 284 new commits pushed to the master branch:

2a37764: 8333743: Change .jcheck/conf branches property to match valid branches
75dc2f8: 8330182: Start of release updates for JDK 24
054362a: 8332550: [macos] Voice Over: java.awt.IllegalComponentStateException: component must be showing on the screen to determine its location
9b436d0: 8333674: Disable CollectorPolicy.young_min_ergo_vm for PPC64
487c477: 8333647: C2 SuperWord: some additional PopulateIndex tests
d02cb74: 8333270: HandlersOnComplexResetUpdate and HandlersOnComplexUpdate tests fail with "Unexpected reference" if timeoutFactor is less than 1/3
02f2404: 8333560: -Xlint:restricted does not work with --release
606df44: 8332670: C1 clone intrinsic needs memory barriers
33fd6ae: 8333622: ubsan: relocInfo_x86.cpp:101:56: runtime error: pointer index expression with base (-1) overflowed
8de5d20: 8332865: ubsan: os::attempt_reserve_memory_between reports overflow
... and 274 more: https://git.openjdk.org/jdk/compare/b92bd671835c37cff58e2cdcecd0fe4277557d7f...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.

openjdk · 2024-05-20T09:06:25Z

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

mlbridge · 2024-05-20T09:09:09Z

Webrevs

00: Full (0c431ef2)

walulyai

Minor suggestion

walulyai · 2024-05-21T07:06:06Z

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

-    }
+void TenuredGeneration::update_promote_stats() {
+  size_t used_after_gc = used();
+  size_t promoted_in_bytes;


Suggested change

size_t promoted_in_bytes;

size_t promoted_in_bytes = 0;

and do away with the else case.

tschatzl · 2024-05-29T08:38:48Z

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

+  if (used_after_gc > _used_at_prologue) {
+    promoted_in_bytes = used_after_gc - _used_at_prologue;
+  } else {
+    promoted_in_bytes = 0;


Question about this new code vs. the old code:

So the changed code updates promotion statistics in old gen also during full gc now. The amount of "promotion" is the difference of used in old gen after and before.

I am not sure this is a good idea as the full gc also reduces the amount of existing old gen live objects. Using 0 in that case (if the full gc collects more than the implicit collection of the young gen during full collection) also seems dodgy at best.

If my reading of the code is correct and there isn't a way to correctly measure the amount of live objects in young gen during full gc I would prefer to just not update the promotion counter during full gc.

Also consider "extra" full gcs like System.gc() calls and other causes for full gc that can happen at any time - they only seem to add noise to the promoted bytes sampling.

The new logic updates promoted-bytes metric on every gc, keeping it up to date at best effort. Not updating it can result into using outdated info in deciding whether to young-gc or full-gc. The bm in https://bugs.openjdk.org/browse/JDK-8320165 shows that one can struck in full-gc forever due to this outdated metric.

Thanks for the clarification.

tschatzl

Lgtm; while you provided performance (internal) neutral benchmark numbers, I would still like to ask you to not push before RDP1 this Thursday to avoid last-minute regressions as it does not seem like an improvement that is necessary for JDK 23.

albertnetymk · 2024-06-07T08:11:31Z

Thanks for review.

/integrate

openjdk · 2024-06-07T08:12:41Z

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

bf7f1c4: 8333211: NMT Reports: replace manual indentation handling with auto indent
8ffc35d: 8333724: Problem list security/infra/java/security/cert/CertPathValidator/certification/CAInterop.java#teliasonerarootcav1
f7862bd: 8331311: C2: Big Endian Port of 8318446: optimize stores into primitive arrays by combining values into larger store
b4beda2: 8332537: C2: High memory usage reported for compiler/loopopts/superword/TestAlignVectorFuzzer.java
e5383d7: 8333713: C2 SuperWord: cleanup in vectornode.cpp/hpp
944aeb8: 8325155: C2 SuperWord: remove alignment boundaries
d8af589: 8026127: Deflater/Inflater documentation incomplete/misleading
6238bc8: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
2a37764: 8333743: Change .jcheck/conf branches property to match valid branches
75dc2f8: 8330182: Start of release updates for JDK 24
... and 282 more: https://git.openjdk.org/jdk/compare/b92bd671835c37cff58e2cdcecd0fe4277557d7f...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-06-07T08:12:47Z

@albertnetymk Pushed as commit a2030ff.

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

s1-promote-stat

0c431ef

openjdk bot changed the title ~~8332516~~ 8332516: Serial: Refactor how promoted bytes are sampled May 20, 2024

openjdk bot added the rfr Pull request is ready for review label May 20, 2024

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label May 20, 2024

walulyai approved these changes May 21, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 21, 2024

tschatzl suggested changes May 29, 2024

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label May 29, 2024

albertnetymk changed the title ~~8332516: Serial: Refactor how promoted bytes are sampled~~ 8332516 May 31, 2024

openjdk bot changed the title ~~8332516~~ 8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs May 31, 2024

openjdk bot added the ready Pull request is ready to be integrated label May 31, 2024

tschatzl approved these changes Jun 3, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2024

openjdk bot closed this Jun 7, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2024

albertnetymk deleted the s1-promote-stat branch June 7, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs #19306

8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs #19306

albertnetymk commented May 20, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 20, 2024

openjdk bot commented May 20, 2024 •

edited

Loading

openjdk bot commented May 20, 2024

mlbridge bot commented May 20, 2024

walulyai left a comment

walulyai May 21, 2024

tschatzl May 29, 2024

albertnetymk May 29, 2024

tschatzl Jun 3, 2024

tschatzl left a comment

albertnetymk commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs #19306

8332516: Serial: Always sample promoted bytes to avoid getting stuck in Full GCs #19306

Conversation

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

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 20, 2024

openjdk bot commented May 20, 2024 • edited Loading

openjdk bot commented May 20, 2024

mlbridge bot commented May 20, 2024

Webrevs

walulyai left a comment

Choose a reason for hiding this comment

walulyai May 21, 2024

Choose a reason for hiding this comment

tschatzl May 29, 2024

Choose a reason for hiding this comment

albertnetymk May 29, 2024

Choose a reason for hiding this comment

tschatzl Jun 3, 2024

Choose a reason for hiding this comment

tschatzl left a comment

Choose a reason for hiding this comment

albertnetymk commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

openjdk bot commented Jun 7, 2024

albertnetymk commented May 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 20, 2024 •

edited

Loading