8308009: Generational ZGC: OOM before clearing all SoftReferences

[fisk]

When a major GC in generational ZGC with a different cause that doesn’t pre-clean and doesn’t clear soft references, we ask if there are allocations stalled on old. And part of that condition is to check if we are not stalled on young. So if an allocation request comes in just before such a “weak” major GC, we will say we won’t clear soft references. But after that major collection we will satisfy all the constraints to throw OOM as both an YC and OC has passed since the allocation request was installed.
The solution is to let the driver remember if it cleared soft references or not, and only throw OOM if it cleared soft references.

---

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-8308009: Generational ZGC: OOM before clearing all SoftReferences

Reviewers

• Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 45b2b269
• Axel Boldt-Christmas (@xmas92 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14122

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@fisk 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]

Webrevs

• 01: Full - Incremental (abd90fc3)
• 00: Full (45b2b269)

[openjdk]

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

8308009: Generational ZGC: OOM before clearing all SoftReferences

Reviewed-by: stefank, aboldtch

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

• 46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
• ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
• dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
• 4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
• 7d2a7ce: 8308672: Add version number in the replay file generated by DumpInline
• 27ba8bd: 8308108: Support Unicode extension for collation settings
• 89b3c37: 8307125: compiler/jvmci/compilerToVM/MaterializeVirtualObjectTest.java hits assert(!Continuation::is_frame_in_continuation(thread(), fr())) failed: No support for deferred values in continuations
• 98acce1: 8306703: JFR: Summary views
• 534de6d: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
• 48d21bd: 8286597: Implement PollerProvider on AIX
• ... and 59 more: https://git.openjdk.org/jdk/compare/c4408278d1012746c91ba4c31068538850c68d30...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.

[xmas92]

I think it looks good.

Only one thing I can think of. There is an implicit assumption here that should_clear_soft_references implies should_preclean_young which is required to be spec compliant. Maybe should_preclean_young should start with

if (should_clear_soft_references(cause)) {
  return true;
}

to make this clearer. Or even short circuit the if (should_preclean_young(...)) directly in ZDriverMajor::collect_young

[fisk]

I think it looks good.

Only one thing I can think of. There is an implicit assumption here that should_clear_soft_references implies should_preclean_young which is required to be spec compliant. Maybe should_preclean_young should start with

if (should_clear_soft_references(cause)) {
  return true;
}

to make this clearer. Or even short circuit the if (should_preclean_young(...)) directly in ZDriverMajor::collect_young

Right. This is already guaranteed because

1. All the causes that trigger soft ref clearing GC, also trigger young precleaning
2. We hold the driver locker between the should_clear_soft_references and should_preclean_young, which means that if soft ref clearing was triggered due to there being a stall, the stall will be there again when we check if we should_preclean_young.

Do we want a comment explaining the above, or change the code structure to be more explicit? I personally don't mind.

[fisk]

I added an assert checking that we don't mess up pre-cleaning and a comment explaining why it holds.

[fisk]

/integrate

[fisk]

Thanks for the review @xmas92!

[openjdk]

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

• bac02b6: 8305189: C2 failed "assert(_outcnt==1) failed: not unique"
• 62537d2: 8299974: Replace NULL with nullptr in share/adlc/
• f09345b: 8308931: Problemlist compiler/jvmci/TestUncaughtErrorInCompileMethod.java
• 7c072db: 8308844: ProblemList gc/z/TestHighUsage.java with Generational ZGC on windows x64
• 4becb7b: 8306137: Open source several AWT ScrollPane related tests
• 199b1bf: 8308583: SIGSEGV in GraphKit::gen_checkcast
• 46c4da7: 8159023: Engineering notation of DecimalFormat does not work as documented
• ee321c7: 8308907: ProblemList java/awt/Toolkit/GetScreenInsetsCustomGC/GetScreenInsetsCustomGC.java on linux-x64
• dc7683a: 8308073: ClassLoaderExt::append_boot_classpath should handle dynamic archive
• 4870234: 8304375: jdk/jfr/api/consumer/filestream/TestOrdered.java failed with "Expected at least some events to be out of order! Reuse = false"
• ... and 65 more: https://git.openjdk.org/jdk/compare/c4408278d1012746c91ba4c31068538850c68d30...master

Your commit was automatically rebased without conflicts.

[openjdk]

@fisk Pushed as commit d3b9b36.

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