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

[Daniel D. Daugherty]

On Wed, 31 Jan 2024 09:04:09 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:
> 
>   More restrictive API

Thumbs up on the technical changes.
I have a few comment nits and don't forget to update copyright years.

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

> 1311: }
> 1312: 
> 1313: ObjectMonitor* ObjectSynchronizer::inflate_impl(LockStack* lock_stack, oop object, const InflateCause cause) {

`inflate_impl` is the new name for the logic that was in `ObjectSynchronizer::inflate()`
and I've crawled thru those changes. I'm happy with this new logic and I'm happy
with passing a `lock_stack` value or nullptr because it allows for some nice refactoring
in the new `inflate_impl` (like getting rid of the static `is_lock_owned` function).

The logic in `ObjectSynchronizer::inflate()` that depended on thecurrent thread being
passed has been refactored:
- ResourceMarks are now created without the thread parameter
- The lock stack manipulations are now dependent on a non-nullptr LockStack being passed instead of a current_thread.

Of course, the callers of `inflate_impl` have to pass a non-nullptr LockStack only when the caller is
the current JavaThread and `LockingMode == LM_LIGHTWEIGHT`.

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

> 1323:     //                   If using fast-locking and the ObjectMonitor owner
> 1324:     //                   is anonymous and the lock_stack contains the
> 1325:     //                   object, then we make the lock_stacks owner the

Nit typo: s/lock_stacks owner/lock_stack's owner/

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

> 1325:     //                   object, then we make the lock_stacks owner the
> 1326:     //                   ObjectMonitor owner and remove the lock from the
> 1327:     //                   lock stack.

nit consistency: s/lock stack/lock_stack/
Other places in the same comment paragraph use an `_`.

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

Marked as reviewed by dcubed (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17626#pullrequestreview-1855004925
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473580684
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473556816
PR Review Comment: https://git.openjdk.org/jdk/pull/17626#discussion_r1473558282