8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure

[tschatzl]

Hi all,

can I have reviews for this change that fixes a benign race between giving the signal to start a new concurrent cycle and G1ConcurrentMarkThread waiting for that signal?

With the changes from JDK-8240556 there is a small window where after the concurrent mark thread sets itself to idle (at the end of the while (wait_for_next_cycle()) loop in G1ConcurrentMarkThread::run_service()), a GC pause may request a new gc, setting the in-progress flag, and the concurrent mark thread waiting for a new cycle in wait_for_next_cycle() where this assert fails.

This ordering can be observed in the log message output:

[342.979s][info ][gc] GC(5240) Pause Remark 75M->75M(132M) 171.410ms
[343.059s][info ][gc] GC(5240) Pause Cleanup 122M->122M(132M) 0.174ms
[343.075s][info ][gc] GC(5241) Pause Young (Concurrent Start) (G1 Humongous Allocation) 123M->123M(132M) 14.023ms <--- sets in-pogress flag
[343.078s][info ][gc] GC(5240) Concurrent Mark Cycle 363.855ms <--- occurs before the next wait_for_next_cycle() call

Previously this problem with the assert did not occur because there has been another "pre-in-progress" state implemented in a somewhat complicated way with nested states. JDK-8240556 removed that one. The messages could be out of order before that change too.

The suggested fix is to remove this assert.

Testing: a few thousand times for that test (not to try to observe the assert which is gone now, but double-check if there is any deadlock etc hiding), tier1-5

---

Progress

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

Issue

• JDK-8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure

Reviewers

• Kim Barrett (@kimbarrett - Reviewer)
• Stefan Johansson (@kstefanj - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/501/head:pull/501
$ git checkout pull/501

[bridgekeeper]

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

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

• 00: Full (8397485)

[openjdk]

@tschatzl 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 more details.

After integration, the commit message for the final commit will be:

8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure

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

• 65cab55: 8253971: ZGC: Flush mark stacks after processing concurrent roots
• 19219a9: 8253960: Memory leak in Java_java_lang_ClassLoader_defineClass0()
• 5d4a135: 8253842: [JVMCI] Allow implicit exception to dispatch to other address in jvmci compilers.
• 289ae79: 8252998: ModuleWrapper.gmk doesn't consult include path
• 9604ee8: 8248238: Implementation: JEP 388: Windows AArch64 Support
• 1c2754b: 8253269: The CheckCommonColors test should provide more info on failure
• d296708: 8253606: Need to add missed constructor to the SwingEventMonitor
• 5810238: 8253945: Missed default constructor for StreamPrintServiceFactory.java
• 123e786: 8239105: Add exception for expiring Digicert root certificates to VerifyCACerts test
• 22f0916: 8253938: ZGC: Clean up argument names after JDK-8253030
• ... and 10 more: https://git.openjdk.java.net/jdk/compare/4185ed3290fa96bd58781082fb777dc75c52bec8...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.

[kstefanj]

Looks good.

[tschatzl]

Thanks for your reviews @kimbarrett @kstefanj

[tschatzl]

/integrate

[openjdk]

@tschatzl Since your change was applied there have been 32 commits pushed to the master branch:

• 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
• 1728547: 8254010: GrowableArrayView::print fails to compile
• 6e61861: 8254046: Remove double semicolon introduced by JDK-8235521
• 5d84e95: 8204256: improve jlink error message to report unsupported class file format
• 4fe68f5: 8253426: jpackage is unable to generate working EXE for add-launcher configurations
• c9d0407: 8253794: TestAbortVMOnSafepointTimeout never timeouts
• f2f77f7: 8253761: Wrong URI syntax printed by jar --describe-module
• b29e108: 8253944: Certain method references to VarHandle methods should fail
• 88d75c9: 8156071: List.of: reduce array copying during creation
• ea27a54: 8224509: Incorrect alignment in CDS related allocation code on 32-bit platforms
• ... and 22 more: https://git.openjdk.java.net/jdk/compare/4185ed3290fa96bd58781082fb777dc75c52bec8...master

Your commit was automatically rebased without conflicts.

Pushed as commit c9d1dcc.

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