8330253: Remove verify_consistent_lock_order #18782

xmas92 · 2024-04-15T09:15:44Z

The verification added in JDK-8329757 will not work when deoptimization occurs on a monitorenter bytecode. The locking may be in a transitional state. Because the correct solution is still not obvious and this test is currently only causing false positives, remove the verification for now.

Redo this verification in JDK-8331307.

Removal tested GHA and tier 1-3.

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-8330253: Remove verify_consistent_lock_order (Bug - P3)

Reviewers

Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to bf49a93f
Patricio Chilano Mateo (@pchilano - Reviewer)
Doug Simon (@dougxc - Reviewer)

Contributors

Patricio Chilano Mateo <pchilanomate@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18782

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-15T09:16:12Z

👋 Welcome back aboldtch! 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-04-15T09:16:46Z

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

8330253: Remove verify_consistent_lock_order

Co-authored-by: Patricio Chilano Mateo <pchilanomate@openjdk.org>
Reviewed-by: dcubed, pchilanomate, dnsimon

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

4e42294: 8329004: Update Libpng to 1.6.43
bdcc240: 8331087: Move immutable nmethod data from CodeCache
8b8fb64: 8324776: runtime/os/TestTransparentHugePageUsage.java fails with The usage of THP is not enough
151ef5d: 8330677: Add Per-Compilation memory usage to JFR
7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
... and 34 more: https://git.openjdk.org/jdk/compare/87e864bf21d71daae4e001ec4edbb4ef1f60c36d...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-04-15T09:17:03Z

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

hotspot

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-04-15T09:37:52Z

Webrevs

src/hotspot/share/runtime/deoptimization.cpp

dean-long · 2024-04-16T02:50:41Z

Can you describe the transitional state this fix avoids, and why it only needs to deal with monitorenter from a synchronized method prologue, and not also monitorenter from synchronized blocks.

dean-long · 2024-04-16T02:54:17Z

Can you describe the transitional state this fix avoids, and why it only needs to deal with monitorenter from a synchronized method prologue, and not also monitorenter from synchronized blocks.

Nevermind, I see that it does handle monitorenter for synchronized methods.

dean-long · 2024-04-16T03:04:49Z

src/hotspot/share/runtime/deoptimization.cpp

+    // If deoptimizing from monitorenter bytecode we maybe in transitional state. Skip verification.
+    if (!is_syncronized_entry && bc != Bytecodes::Code::_monitorenter) {
+      deoptee_thread->lock_stack().verify_consistent_lock_order(lock_order, exec_mode != Deoptimization::Unpack_none);
+    }


The above checks would also hit the follow false positives:

deopt in counter overflow in prologue, not in monitorenter

monitorenter at bci 0 when raw_bci is -1 (assuming it got past the verifier)
but seems mostly harmless to skip checks in those cases.

I thought the original check was fine. Could you elaborate on these 2 cases, I didn't really get them.

Deoptimization is already expensive, and this edge case is rare, so I think it would be better to compute the actual previous bytecode here, and not use bci - 1.

Other debug checks in deoptimization compute oop maps, which have to iterate all the bytecodes, so doing it here also wouldn't be so bad. Or how about not checking bytecodes and instead checking a flag on JavaThread that says we are in monitor enter native code?

Deoptimization is already expensive, and this edge case is rare, so I think it would be better to compute the actual previous bytecode here, and not use bci - 1.

Alright I will give that a go then, unless we think the second option is more appropriate.

Or how about not checking bytecodes and instead checking a flag on JavaThread that says we are in monitor enter native code?

What state would that be? Any current state we setup is after transitioning to VM which seems to late. I guess it would be possible to instrument SharedRuntime::monitor_enter_helper with some thread local state we can check. But the interpreter has its own entry point. But I guess we would never reach this point for the interpreter, as there is no such thing as deoptimizing or unpacking interpreted frames.

What is the feeling here? What would be more appropriate? Adding some thread local debug state that we set before transitioning to VM in SharedRuntime::monitor_enter_helper seems like the most precise solution, but we need to be sure that there are no earlier safepoint polls before this point within the execution of the monitorenter bytecode.

I see there is _pending_monitorenter but this would only handle synchronized method entry.

I like the idea of a flag better, because it is foolproof. Why can't we set it in ObjectSynchronizer::enter? I don't think it matters if there is a safepoint check before that, because the lock stack is still consistent at that point.

(Currently) the lock stack is always consistent at safepoints w.r.t. what is actually locked. However the lock stack may not be consistent with the most recent lock returned by the leaf compiledVFrame::monitors().

But you are correct that it can probably be moved to ObjectSynchronizer::enter there are no safepoint polls between SharedRuntime::monitor_enter_helper and that point. Similarly there are no safepoints polls in the runtime until after set_current_pending_monitor is called.

So with these following assumptions.

LockStack is consistent at safepoints w.r.t. locked monitors

No safepoint polls exist from the point that compiledVFrame::monitors() starts returning the monitorinfo for the currently executing monitorenter until either it calls into the runtime or finishes locking.

I do not believe 1. is likely to ever change. But I have limited understanding of the validity of 2. nor if it something that can change.

If both these assumptions are correct than simply skipping the verification when deoptee_thread->current_pending_monitor() != nullptr would suffice.

I believe those assumptions will always hold, but per separate discussions, let's go with what you have.

dcubed-ojdk · 2024-04-17T16:47:25Z

Please clarify what pre-integration testing is being done. As far as I can
tell, this failure only shows up in Tier8 so that should be part of your mix.

src/hotspot/share/runtime/deoptimization.cpp

dcubed-ojdk

Thumbs up on the changes themselves.

Unless I missed it, I haven't seen updated info on the pre-integration testing
in general and specifically about whether Tier8 was executed.

pchilano · 2024-04-18T19:15:15Z

I was playing with a reproducer, maybe it would be good to add it: pchilano@77a85a0

xmas92 · 2024-04-19T05:22:31Z

Thumbs up on the changes themselves.

Unless I missed it, I haven't seen updated info on the pre-integration testing in general and specifically about whether Tier8 was executed.

Just finished Tier8. Also stress tested the reproducer (both @pchilano and the reproducers from Tier8)

I was playing with a reproducer, maybe it would be good to add it: pchilano@77a85a0

Great thanks, I'll add it.

/contributor add @pchilano

openjdk · 2024-04-19T10:06:07Z

@xmas92
Contributor Patricio Chilano Mateo <pchilanomate@openjdk.org> successfully added.

This reverts commit 10d70ea.

xmas92 · 2024-04-29T13:45:46Z

There were some additional issues with 10d70ea that has to be resolved before this could go in. And multiple engineers in this area agree that the bytecode filter is probably not the way to solve this. JDK-8331307 has been created to redo verify_consistent_lock_order.

Because this issue is creating false positives in testing I propose that we first remove the verification so it can be redone.

Reverted the test and removed the verification logic.

Added tag jdk-23+20 for changeset 87e864b

xmas92 · 2024-04-29T17:13:48Z

Thanks for the reviews.

/integrate

openjdk · 2024-04-29T17:14:10Z

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

4e42294: 8329004: Update Libpng to 1.6.43
bdcc240: 8331087: Move immutable nmethod data from CodeCache
8b8fb64: 8324776: runtime/os/TestTransparentHugePageUsage.java fails with The usage of THP is not enough
151ef5d: 8330677: Add Per-Compilation memory usage to JFR
7272939: 8331200: Serial: Remove unused methods in SerialHeap
70d3f22: 8331175: Parallel: Remove VerifyRememberedSets
549bc6a: 8330685: ZGC: share barrier spilling logic
76cda7b: 8331118: Remove Serial includes from space.hpp
766d0b0: 8330960: Serial: Remove SerialFullGC::_total_invocations
4edac34: 8331167: UBSan enabled build fails in adlc on macOS
... and 34 more: https://git.openjdk.org/jdk/compare/87e864bf21d71daae4e001ec4edbb4ef1f60c36d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-29T17:14:16Z

@xmas92 Pushed as commit 9b423a8.

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

Skip verify when deoptimizing from monitorenter bytecode.

4e00138

openjdk bot changed the title ~~8330164~~ 8330164: Apr 15, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 15, 2024

xmas92 changed the title ~~8330164:~~ 8330253 Apr 15, 2024

openjdk bot changed the title ~~8330253~~ 8330253: Skip verify_consistent_lock_order when deoptimizing from monitorenter bytecode. Apr 15, 2024

xmas92 marked this pull request as ready for review April 15, 2024 09:34

openjdk bot added the rfr Pull request is ready for review label Apr 15, 2024

vnkozlov reviewed Apr 15, 2024

View reviewed changes

src/hotspot/share/runtime/deoptimization.cpp Outdated Show resolved Hide resolved

dean-long reviewed Apr 16, 2024

View reviewed changes

xmas92 added 2 commits April 16, 2024 12:02

Remove implicit conditions

6b416ab

Handle previous bc being monitorenter

03a4e04

dcubed-ojdk suggested changes Apr 17, 2024

View reviewed changes

xmas92 added 2 commits April 18, 2024 05:34

Spelling and typos

e661124

Whitespace

bf49a93

dcubed-ojdk approved these changes Apr 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 18, 2024

repro for JDK-8330253

10d70ea

xmas92 added 3 commits April 22, 2024 05:32

Use raw bytecode read for previous bytecode

8d815e9

Fix condition

578a832

Revert "repro for JDK-8330253"

5dc00fd

This reverts commit 10d70ea.

Revert verification

be0a688

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

xmas92 changed the title ~~8330253: Skip verify_consistent_lock_order when deoptimizing from monitorenter bytecode.~~ 8330253: Remove verify_consistent_lock_order Apr 29, 2024

openjdk bot added the ready Pull request is ready to be integrated label Apr 29, 2024

Merge tag 'jdk-23+20' into JDK-8330164

d913ecb

Added tag jdk-23+20 for changeset 87e864b

pchilano approved these changes Apr 29, 2024

View reviewed changes

dougxc approved these changes Apr 29, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 29, 2024

openjdk bot closed this Apr 29, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8330253: Remove verify_consistent_lock_order #18782

8330253: Remove verify_consistent_lock_order #18782

xmas92 commented Apr 15, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 15, 2024

openjdk bot commented Apr 15, 2024 •

edited

Loading

openjdk bot commented Apr 15, 2024

mlbridge bot commented Apr 15, 2024 •

edited

Loading

dean-long commented Apr 16, 2024

dean-long commented Apr 16, 2024

dean-long Apr 16, 2024

pchilano Apr 18, 2024

dean-long Apr 22, 2024

dean-long Apr 22, 2024

xmas92 Apr 23, 2024

xmas92 Apr 23, 2024

dean-long Apr 23, 2024

xmas92 Apr 23, 2024 •

edited

Loading

dean-long Apr 23, 2024

dcubed-ojdk commented Apr 17, 2024

dcubed-ojdk left a comment

pchilano commented Apr 18, 2024

xmas92 commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

xmas92 commented Apr 29, 2024

xmas92 commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

8330253: Remove verify_consistent_lock_order #18782

8330253: Remove verify_consistent_lock_order #18782

Conversation

xmas92 commented Apr 15, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

Webrev

bridgekeeper bot commented Apr 15, 2024

openjdk bot commented Apr 15, 2024 • edited Loading

openjdk bot commented Apr 15, 2024

mlbridge bot commented Apr 15, 2024 • edited Loading

Webrevs

dean-long commented Apr 16, 2024

dean-long commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmas92 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcubed-ojdk commented Apr 17, 2024

dcubed-ojdk left a comment

Choose a reason for hiding this comment

pchilano commented Apr 18, 2024

xmas92 commented Apr 19, 2024

openjdk bot commented Apr 19, 2024

xmas92 commented Apr 29, 2024

xmas92 commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

openjdk bot commented Apr 29, 2024

xmas92 commented Apr 15, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 15, 2024 •

edited

Loading

mlbridge bot commented Apr 15, 2024 •

edited

Loading

xmas92 Apr 23, 2024 •

edited

Loading