On Mon, 19 Sep 2022 20:41:50 GMT, Chris Plummer <[email protected]> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fixed typo in VirtualThread.c
>
> src/hotspot/share/runtime/javaThread.cpp line 1174:
>
>> 1172: #if INCLUDE_JVMTI
>> 1173: // Suspending a JavaThread in VTMS transition or disabling VTMS
>> transitions can cause deadlocks.
>> 1174: assert(!is_in_non_tmp_VTMS_transition(), "no suspend allowed in VTMS
>> transition");
>
> IMHO, a non tmp VTMS transition should be considered a type of VTMS
> transition, so the assert check here does not really match the assert text.
> Also see my related comments below on naming and use of these flags and APIs.
Thanks. Accepted.
> src/hotspot/share/runtime/javaThread.hpp line 650:
>
>> 648: void set_is_in_VTMS_transition(bool val);
>> 649: bool is_in_tmp_VTMS_transition() const { return
>> _is_in_tmp_VTMS_transition; }
>> 650: void toggle_is_in_tmp_VTMS_transition() {
>> _is_in_tmp_VTMS_transition = !_is_in_tmp_VTMS_transition; };
>
> The naming of the flags does not match up with the naming of the APIs, which
> is confusing. An API named is_in_VTMS_transition() should return
> _is_in_VTMS_transition. I think part of problem is that
> _is_in_VTMS_transition is not a superset of _is_in_tmp_VTMS_transition. The
> flags are never both set true, although both can be false. Maybe this should
> be changed to make it an invariant that if _is_in_tmp_VTMS_transit is true,
> then _is_in_tmp_VTMS_transition is true.
I agree, this naming is not good. However, I was reluctant to do required
renaming because it can potentially impact many spots and make review harder.
Let me think a little bit more on this.
-------------
PR: https://git.openjdk.org/jdk/pull/10321
