On Tue, 20 Sep 2022 22:41:50 GMT, Serguei Spitsyn <[email protected]> wrote:
>> 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.
Chris, I believe, this comment is resolved. Please, let me know if you disagree.
>> 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.
I've simplified this naming but, probably, not in a way your were expecting.
Please, see my comment below.
-------------
PR: https://git.openjdk.org/jdk/pull/10321
