RFR: 8324881: ObjectSynchronizer::inflate(Thread* current...) is invoked for non-current thread [v10]

[Daniel D. Daugherty]

On Mon, 5 Feb 2024 06:51:22 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> The `ObjectSynchronizer` has always assumed that the `current` parameters are both the current thread as well as the thread that is doing the locking. The only time that we are entering on behalf of another thread is when doing re-locking in deoptimization. This has worked because the deoptee thread is suspended. However ResourceMarks have been using the wrong thread when logging is enabled.
>> 
>> This change `ObjectSynchronizer` instruments the relevant methods with both a `JavaThread* locking_thread` as well as `[Java]Thread* current` to be able to use the correct thread for ResourceMarks. 
>> 
>> Having the `inflate` care about a `locking_thread` is a little unpleasant in my opinion. But it is required for LM_LIGHTWEIGHT. 
>> Would probably be cleaner if the inflate for LM_LIGHTWEIGHT was it's own thing, as it does not share the whole INFLATING protocol. But seems like a future RFE to refactor this code.
>> 
>> Can reproduce a crash by modifying `test/jdk/com/sun/jdi/EATests.java` and using `-XX:DiagnoseSyncOnValueBasedClasses=2` with LM_LEGACY or running `test/jdk/com/sun/jdi/EATests.java` with LM_LIGHTWEIGHT/LM_MONITOR and `-Xlog:monitorinflation=trace`. 
>> 
>> Could extend this test to capture this regression in the future (or creating a new test based on the same infrastructure). Will give it an attempt, so we have a regression test for this. But these tests get rather involved as the require a lot of jvmti setup.
>
> Axel Boldt-Christmas has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 22 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream_jdk/master' into JDK-8324881
>  - Update comment
>  - Fix comments
>  - Update test/jdk/com/sun/jdi/EATests.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Update test/jdk/com/sun/jdi/EATests.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Update test/jdk/com/sun/jdi/EATests.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Update test/jdk/com/sun/jdi/EATests.java
>    
>    Co-authored-by: Andrey Turbanov <turbanoff at gmail.com>
>  - Remove newline
>  - Update src/hotspot/share/runtime/synchronizer.hpp
>    
>    Co-authored-by: Richard Reingruber <richard.reingruber at sap.com>
>  - Improve EARelockingNestedInflated_03 for LM_LIGHTWEIGHT
>  - ... and 12 more: https://git.openjdk.org/jdk/compare/71bd9124...b3cc401c

I previously reviewed the V04 full webrev and this time I reviewed the v09 full webrev. Thumbs up. I have just one question about an assert below, a comment suggestion and a spacing nit.

Very nice job on this gnarly fix.

src/hotspot/share/runtime/synchronizer.cpp line 525:

> 523:         return;
> 524:       }
> 525:       assert(monitor->is_being_async_deflated(), "must be");

You've added this assertion here after the `enter_for` must have lost the
race to async deflation, but you didn't add the same assertion below
after L540 -> L542. Why not?

src/hotspot/share/runtime/synchronizer.cpp line 546:

> 544:           return;
> 545:         }
> 546:         // Fall through to inflate() ...

You removed this comment here, but there is also this comment:
L578: // All other paths fall-through to inflate-enter.

For both of those, I would have updated to say something about
falling through to returning false instead of deletion.

test/jdk/com/sun/jdi/EATests.java line 2019:

> 2017:                 }
> 2018:             }
> 2019:             synchronized(lockInflatedByContention) { // will block and trigger inflation

nit space after `synchronized`.

-------------

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17626#pullrequestreview-1866519912
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1480662827
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1480665913
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1480670313