8307236: Rendezvous GC threads under STS for monitor deflation #13773

rkennke · 2023-05-03T10:06:43Z

Object monitors are deflated concurrently by the MonitorDeflationThread. It first unlinks monitors from objects (i.e. restore the original object header), then handshakes (with a no-op) all Java threads, and only then destroys the monitors. This way, Java threads can safely (and racily) access monitors before the handshake, because the monitors are guaranteed to still exist when a Java thread racily reads a mark-word that is being unlinked, and the monitor can safely be destroyed after the handshake, because all Java threads would then read the correct unlinked mark-word.

However, GC threads are not rendezvous'ed like that, and can read potentially dead monitors. At least with the upcoming Compact Object Headers this is going to be a problem, because the compressed Klass* is then part of the mark-word.

In order to safely access monitors via object headers concurrently from GC threads, we need to rendezvous them after unlinking and before destroying the monitors, just like Java threads do, via handshake. This is important so that concurrent GCs (ZGC, Shenandoah, G1) can safely access object's Klass* (and thus object size, layout, etc) during concurrent GC phases.

This only implements the parts that do the rendezvous, it still requires that affected concurrent GC threads are under SustainableThreadSet (most of them already are!). This will be implemented in later PR.

Testing:

tier1
tier2

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-8307236: Rendezvous GC threads under STS for monitor deflation

Reviewers

Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to 68357ae2
Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13773

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-03T10:07:21Z

👋 Welcome back rkennke! 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 · 2023-05-03T10:09:54Z

@rkennke The following label will be automatically applied to this pull request:

hotspot-runtime

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 · 2023-05-03T13:10:21Z

Webrevs

fisk

Looks good.

openjdk · 2023-05-03T13:13:15Z

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

8307236: Rendezvous GC threads under STS for monitor deflation

Reviewed-by: eosterlund, shade

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:

f94f957: 8307331: Correctly update line maps when class redefine rewrites bytecodes
302bc2f: 8307421: Fix comment in g1CollectionSetChooser.hpp after JDK-8306836
e19cf26: 8307196: Dangling pointer warning for MetadataAllocationRequest
1b143ba: 8307378: Allow collectors to provide specific values for GC notifications' actions
a44e890: 8307308: Add serviceability_ttf_virtual group to exclude jvmti tests developed for virtual threads
46df171: 8304948: [vectorapi] C2 crashes when expanding VectorBox
111858f: 8307489: ProblemList jdk/incubator/vector/LoadJsvmlTest.java on windows-x64
197d0cc: 8294983: SSLEngine throws ClassCastException during handshake
a87262e: 8307295: Add warning to not create new ACC flags
0c6529d: 8307156: native_thread not protected by TLH
... and 23 more: https://git.openjdk.org/jdk/compare/fcb280a48bf9f562e6c0982c1d7a0076ee2e736e...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.

shipilev · 2023-05-04T08:19:25Z

src/hotspot/share/runtime/synchronizer.cpp

+      // Also, we sync and desync GC threads around the handshake, so that they can
+      // safely read the mark-word and look-through to the object-monitor, without
+      // being afraid that the object-monitor is going away.
+      VM_RendezvousGCThreads sync_gc;
+      VMThread::execute(&sync_gc);


Not sure why this block is under current->is_Java_thread() block. Shouldn't we rendezvous with GC threads regardless?

Not sure why this block is under current->is_Java_thread() block. Shouldn't we rendezvous with GC threads regardless?

Deflation is done either concurrently by the service thread, or in a safepoint by the VM thread. The current->is_Java_thread() means it's the concurrent case from the service thread. That's when it's needed. Inside of safepoints, the GC threads have already been synchronized.

Okay, I see. Can we then move this block into separate current->is_service_thread()-checked block, and assert(current->is_VM_thread()) on else branch? This would make this distinction very clear.

shipilev · 2023-05-04T11:05:09Z

src/hotspot/share/runtime/synchronizer.cpp


      if (ls != nullptr) {
        ls->print_cr("after handshaking: in_use_list stats: ceiling="
                     SIZE_FORMAT ", count=" SIZE_FORMAT ", max=" SIZE_FORMAT,
                     in_use_list_ceiling(), _in_use_list.count(), _in_use_list.max());
        timer.start();
      }
+    } else {
+      assert(current->is_VM_thread(), "only VM thread may get here");


Actually, maybe we should assert we are at safepoint? To protect the accidental call from the VMOp that is executed by VMThread, but outside of safepoint.

} else { // This is not a monitor deflation thread. // No handshake or rendezvous is needed when we are already at safepoint. assert_at_safepoint(); }

shipilev

This looks fine to me. @fisk and/or @dcubed-ojdk might want to re-review :)

src/hotspot/share/runtime/monitorDeflationThread.hpp

rkennke · 2023-05-05T14:33:41Z

Thanks!
/integrate

openjdk · 2023-05-05T14:35:39Z

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

6fe959c: 8307306: Change some ConstantPool::name_ref_at calls to uncached_name_ref_at
3f6a354: 8305169: java/security/cert/CertPathValidator/OCSP/GetAndPostTests.java -- test server didn't start in timely manner
f143bf7: 8305084: Remove the removal warnings for finalize() from test/hotspot/jtreg/serviceability/dcmd/gc/FinalizerInfoTest.java and RunFinalizationTest.java
746f8d1: 8305714: Add an extra test for JDK-8292755
1a1ce66: 8305080: Suppress the 'removal' warning for finalize() from test/hotspot/jtreg/compiler/jvmci/common/testcases that used in compiler/jvmci/compilerToVM/ tests
3b430b9: 8250596: Update remaining manpage references from "OS X" to "macOS"
948f3b3: 8301493: Replace NULL with nullptr in cpu/aarch64
f94f957: 8307331: Correctly update line maps when class redefine rewrites bytecodes
302bc2f: 8307421: Fix comment in g1CollectionSetChooser.hpp after JDK-8306836
e19cf26: 8307196: Dangling pointer warning for MetadataAllocationRequest
... and 30 more: https://git.openjdk.org/jdk/compare/fcb280a48bf9f562e6c0982c1d7a0076ee2e736e...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-05T14:35:49Z

@rkennke Pushed as commit 12d6ec6.

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

8307236: Rendezvous GC threads under STS for monitor deflation

e3ff03a

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 3, 2023

rkennke marked this pull request as ready for review May 3, 2023 13:04

openjdk bot added the rfr Pull request is ready for review label May 3, 2023

fisk approved these changes May 3, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 3, 2023

shipilev reviewed May 4, 2023

View reviewed changes

Check for monitor-deflation or VM-thread

39c57a7

shipilev reviewed May 4, 2023

View reviewed changes

Assert for safepoint instead of VMThread

68357ae

shipilev approved these changes May 4, 2023

View reviewed changes

fisk approved these changes May 4, 2023

View reviewed changes

shipilev reviewed May 4, 2023

View reviewed changes

src/hotspot/share/runtime/monitorDeflationThread.hpp Outdated Show resolved Hide resolved

Remove override

e8a73cf

shipilev approved these changes May 5, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 5, 2023

openjdk bot closed this May 5, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8307236: Rendezvous GC threads under STS for monitor deflation #13773

8307236: Rendezvous GC threads under STS for monitor deflation #13773

rkennke commented May 3, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 3, 2023

openjdk bot commented May 3, 2023

mlbridge bot commented May 3, 2023 •

edited

Loading

fisk left a comment

openjdk bot commented May 3, 2023 •

edited

Loading

shipilev May 4, 2023

fisk May 4, 2023

shipilev May 4, 2023

shipilev May 4, 2023

shipilev left a comment •

edited

Loading

rkennke commented May 5, 2023

openjdk bot commented May 5, 2023

openjdk bot commented May 5, 2023

8307236: Rendezvous GC threads under STS for monitor deflation #13773

8307236: Rendezvous GC threads under STS for monitor deflation #13773

Conversation

rkennke commented May 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented May 3, 2023

openjdk bot commented May 3, 2023

mlbridge bot commented May 3, 2023 • edited Loading

Webrevs

fisk left a comment

Choose a reason for hiding this comment

openjdk bot commented May 3, 2023 • edited Loading

shipilev May 4, 2023

Choose a reason for hiding this comment

fisk May 4, 2023

Choose a reason for hiding this comment

shipilev May 4, 2023

Choose a reason for hiding this comment

shipilev May 4, 2023

Choose a reason for hiding this comment

shipilev left a comment • edited Loading

Choose a reason for hiding this comment

rkennke commented May 5, 2023

openjdk bot commented May 5, 2023

openjdk bot commented May 5, 2023

rkennke commented May 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 3, 2023 •

edited

Loading

openjdk bot commented May 3, 2023 •

edited

Loading

shipilev left a comment •

edited

Loading