8194823: Serial GC does not account GCs caused by TLAB allocation in GC overhead limit

[lgxbslgx]

Hi all,

This patch enables the gc overhead limit when allocating TLAB in serial gc.
The main modification is at GenCollectedHeap::allocate_new_tlab and the other
files only adjust the parameters of the method allocate_new_tlab.

Thanks for the review.

Best Regards,
-- Guoxiong

---

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-8194823: Serial GC does not account GCs caused by TLAB allocation in GC overhead limit

Reviewers

• Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to f19d3213
• Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14120

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back gli! 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]

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

• hotspot-gc
• 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]

Webrevs

• 01: Full - Incremental (fd9b3969)
• 00: Full (f19d3213)

[tschatzl]

Lgtm apart from parameter list formatting issues. Please fix before committing.

It's a bit unfortunate that support for this feature for one collector causes so many changes everywhere else. (But also indicates an issue with all other collectors not supporting it ;)). Maybe the parameters could be wrapped into something like an "AllocationRequest", but this is a) a separate issue, and b) needs to be discussed first with others.

[openjdk]

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

8194823: Serial GC does not account GCs caused by TLAB allocation in GC overhead limit

Reviewed-by: tschatzl, 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 33 new commits pushed to the master branch:

• 90e57fd: 8308335: JFR: Remove @experimental from Virtual Threads events
• 7e2e05d: 8308098: G1: Remove redundant checks in G1ObjectCountIsAliveClosure
• 2599ada: 8308655: Narrow types of ConstantPool and ConstMethod returns
• 5a0a238: 8308746: C2 IR test failures for TestFpMinMaxReductions.java with SSE2
• 38367d3: 8308735: Typos in parameter names
• 4f096eb: 8305635: Replace Parse Predicate IfNode with new ParsePredicateNode and route predicate queries through dedicated classes
• f27bc59: 8307132: Cleanup the code of sun.java2d.cmm.lcms package
• 426ebf4: 8308475: Make the thread dump files generated by jcmd Thread.dump_to_file jtreg failure handler action easily accessible
• 8d8153e: 8307958: Metaspace verification is slow causing extreme class unloading times
• d877134: 8144891: ToolBox should use java.nio.file.Path internally, instead of java.io.File
• ... and 23 more: https://git.openjdk.org/jdk/compare/466ec300fc8e5702553123cf2fa4b0d8c7d552d9...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.

[lgxbslgx]

@tschatzl Thanks for the review. I fixed the formatting issues just now.

[albertnetymk]

Do you have a small example to trigger java.lang.OutOfMemoryError: GC Overhead Limit Exceeded for Serial GC. I was under the impression that Serial doesn't support UseGCOverheadLimit.

[lgxbslgx]

Do you have a small example to trigger java.lang.OutOfMemoryError: GC Overhead Limit Exceeded for Serial GC. I was under the impression that Serial doesn't support UseGCOverheadLimit.

I re-read the related code and now I think you are right.

Currently, only the parallel GC supports UseGCOverheadLimit.
In detail, the methods GCOverheadChecker::check_gc_overhead_limit
and AdaptiveSizePolicy::check_gc_overhead_limit are only used by
PSScavenge::invoke_no_policy and PSParallelCompact::invoke_no_policy
under the condition UseAdaptiveSizePolicy is true.
And the UseAdaptiveSizePolicy is only used by paralled gc, too.

Several problems need to be confirmed before continuing the work:

The UseGCOverheadLimit is only used when UseAdaptiveSizePolicy is true. Is it intentional?
If it is intentional and only the parallel GC uses the UseAdaptiveSizePolicy now,
should I remove the UseGCOverheadLimit related code in serial GC?
Or we should implement the feature about UseAdaptiveSizePolicy
and UseGCOverheadLimit in serial GC (seems a large change) ?

[albertnetymk]

The UseGCOverheadLimit is only used when UseAdaptiveSizePolicy is true. Is it intentional?

I can't see any dependency btw them after skimming through their specification. Worth investigation in its own ticket/PR.

Some background which might be useful, "UseGCOverheadLimit can work independently of UseAdaptiveSizePolicy" from https://bugs.openjdk.org/browse/JDK-8212206

should I remove the UseGCOverheadLimit related code in serial GC?

I am leaned towards removing it, as it is effectively dead code.

[lgxbslgx]

I can't see any dependency btw them after skimming through their specification. Worth investigation in its own ticket/PR.

Some background which might be useful, "UseGCOverheadLimit can work independently of UseAdaptiveSizePolicy" from https://bugs.openjdk.org/browse/JDK-8212206

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

I am leaned towards removing it, as it is effectively dead code.

I also think it is good to remove it.
If hearing no objection in the next several days, I will submit another issue&PR to remove it and close this one.
Or I should remove it in this PR? Need to be confirmed here.

[albertnetymk]

I think it's best to do the removing in another ticket/PR and link the two tickets.

[lgxbslgx]

I think it's best to do the removing in another ticket/PR and link the two tickets.

Filed https://bugs.openjdk.org/browse/JDK-8309265 to follow up. Closing this one.