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

[Daniel D. Daugherty]

On Thu, 1 Feb 2024 12:40:26 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 incrementally with one additional commit since the last revision:
> 
>   Fix enter_for LockingMode == LM_LEGACY and strengthen assert

Thumbs up modulo other reviews comments being resolved.

Please don't forget to update copyright years.

src/hotspot/share/runtime/objectMonitor.cpp line 318:

> 316: 
> 317: bool ObjectMonitor::enter_for(JavaThread* locking_thread) {
> 318:   // Used by ObjectSynchronizer::enter_for to enter for another thread

style bit: please end this sentence with a `;` and the lower case start of the next sentence will make sense.

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

> 508: // The interpreter and compiler assembly code tries to lock using the fast path
> 509: // of this algorithm. Make sure to update that code if the following function is
> 510: // changed. The implementation is extremely sensitive to race condition. Be careful.

This comment belongs before `enter_fast_impl`.

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

> 1345:   // important for the correctness of the LM_LIGHTWEIGHT algorithm that the thread
> 1346:   // is set when called from ObjectSynchronizer::enter from the owning thread,
> 1347:   // ObjectSynchronizer::enter_for from any thread, or ObjectSynchronizer::exit

nit typo: please end this sentence with a `.`

src/hotspot/share/runtime/synchronizer.hpp line 105:

> 103: private:
> 104:   // Shared implementation for enter and enter_for. Preforms all but
> 105:   // inflated monitor enter

nit typo: s/Preforms/ Performs/
Please end the sentence with a `.`

src/hotspot/share/runtime/synchronizer.hpp line 124:

> 122:   // Inflate light weight monitor to heavy weight monitor
> 123:   static ObjectMonitor* inflate(Thread* current, oop obj, const InflateCause cause);
> 124:   // Used inflate a monitor as if it was done from the thread JavaThread.

nit typo: s/Used inflate/Used to inflate/

src/hotspot/share/runtime/synchronizer.hpp line 128:

> 126: 
> 127: private:
> 128:   // Shared implementation between the different LockingMode

nit typo: Please end the sentence with a .

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

> 146:  *                 -XX:+DoEscapeAnalysis -XX:+EliminateAllocations -XX:+EliminateLocks -XX:+EliminateNestedLocks
> 147:  *                 -XX:LockingMode=0
> 148:  *                 -Xlog:monitorinflation=trace

You might want to try this:

-Xlog:monitorinflation=trace:file=monitorinflation.log

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17626#pullrequestreview-1857650292
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475160212
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475178393
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475183858
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475188239
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475188966
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475189723
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1475194756