On Wed, 30 Oct 2024 17:23:31 GMT, Coleen Phillimore <cole...@openjdk.org> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Fix in JvmtiEnvBase::get_locked_objects_in_frame()
>> - Add ObjectWaiter::at_monitorenter
>
> src/hotspot/share/oops/stackChunkOop.inline.hpp line 189:
>
>> 187: inline ObjectMonitor* stackChunkOopDesc::current_pending_monitor()
>> const {
>> 188: ObjectWaiter* waiter = object_waiter();
>> 189: if (waiter != nullptr && (waiter->is_monitorenter() ||
>> (waiter->is_wait() && (waiter->at_reenter() || waiter->notified())))) {
>
> Can we hide this conditional under ObjectWaiter::pending_monitor() { all this
> stuff with a comment; }
>
> Not sure what this is excluding.
I added method `at_monitorenter()` to ObjectWaiter. We are checking if the
vthread is blocked trying to acquire (or re-acquire for the wait case) the
monitor. While looking at these I also noticed we were missing a call to
`current_waiting_monitor` in `JvmtiEnvBase::get_locked_objects_in_frame()` so I
added it. We didn’t had a case for this so it went unnoticed. I extended JVMTI
test `VThreadMonitorTest.java` to cover this case.
> src/hotspot/share/runtime/continuationFreezeThaw.cpp line 1657:
>
>> 1655: }
>> 1656:
>> 1657: template<typename ConfigT, bool preempt>
>
> This function is kind of big, do we really want it duplicated to pass preempt
> as a template parameter?
I checked and release build is same size and fast/slow debug difference is only
about 16kb. Since it doesn’t hurt I would rather not touch the fast paths, but
I see `ConfigT` has been unused for some time now so I can do a follow up
cleanup.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1834427410
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1834425311
