On Tue, 22 Oct 2024 06:31:47 GMT, David Holmes <dhol...@openjdk.org> wrote:
>> Patricio Chilano Mateo has updated the pull request incrementally with six
>> additional commits since the last revision:
>>
>> - Fix comments in objectMonitor.hpp
>> - Move frame::saved_thread_address() to platform dependent files
>> - Fix typo in jvmtiExport.cpp
>> - remove usage of frame::metadata_words in possibly_adjust_frame()
>> - Fix comments in c2 locking paths
>> - Revert and simplify changes to c1_Runtime1 on aarch64 and riscv
>
> src/hotspot/share/runtime/objectMonitor.hpp line 292:
>
>> 290:
>> 291: static int64_t owner_for(JavaThread* thread);
>> 292: static int64_t owner_for_oop(oop vthread);
>
> Some comments describing this API would be good. I'm struggling a bit with
> the "owner for" terminology. I think `owner_from` would be better. And can't
> these just overload rather than using different names?
I changed them to `owner_from`. I added a comment referring to the return value
as tid, and then I used this tid name in some other comments. Maybe this
methods should be called `tid_from()`? Alternatively we could use the term
owner id instead, and these would be `owner_id_from()`. In theory, this tid
term or owner id (or whatever other name) does not need to be related to
`j.l.Thread.tid`, it just happens that that's what we are using as the actual
value for this id.
> src/hotspot/share/runtime/objectMonitor.hpp line 302:
>
>> 300: // Simply set _owner field to new_value; current value must match
>> old_value.
>> 301: void set_owner_from_raw(int64_t old_value, int64_t new_value);
>> 302: void set_owner_from(int64_t old_value, JavaThread* current);
>
> Again some comments describing API would good. The old API had vague names
> like old_value and new_value because of the different forms the owner value
> could take. Now it is always a thread-id we can do better I think. The
> distinction between the raw and non-raw forms is unclear and the latter is
> not covered by the initial comment.
I added a comment. How about s/old_value/old_tid and s/new_value/new_tid?
> src/hotspot/share/runtime/objectMonitor.hpp line 303:
>
>> 301: void set_owner_from_raw(int64_t old_value, int64_t new_value);
>> 302: void set_owner_from(int64_t old_value, JavaThread* current);
>> 303: // Simply set _owner field to current; current value must match
>> basic_lock_p.
>
> Comment is no longer accurate
Fixed.
> src/hotspot/share/runtime/objectMonitor.hpp line 309:
>
>> 307: // _owner field. Returns the prior value of the _owner field.
>> 308: int64_t try_set_owner_from_raw(int64_t old_value, int64_t
>> new_value);
>> 309: int64_t try_set_owner_from(int64_t old_value, JavaThread* current);
>
> Similar to set_owner* need better comments describing API.
Added similar comment.
> src/hotspot/share/runtime/objectMonitor.hpp line 311:
>
>> 309: int64_t try_set_owner_from(int64_t old_value, JavaThread* current);
>> 310:
>> 311: bool is_succesor(JavaThread* thread);
>
> I think `has_successor` is more appropriate here as it is not the monitor
> that is the successor.
Right, changed.
> src/hotspot/share/runtime/objectMonitor.hpp line 315:
>
>> 313: void set_succesor(oop vthread);
>> 314: void clear_succesor();
>> 315: bool has_succesor();
>
> Sorry but `successor` has two `s` before `or`.
Fixed.
> src/hotspot/share/runtime/objectMonitor.hpp line 317:
>
>> 315: bool has_succesor();
>> 316:
>> 317: bool is_owner(JavaThread* thread) const { return owner() ==
>> owner_for(thread); }
>
> Again `has_owner` seems more appropriate
Yes, changed.
> src/hotspot/share/runtime/objectMonitor.hpp line 323:
>
>> 321: }
>> 322:
>> 323: bool is_owner_anonymous() const { return owner_raw() ==
>> ANONYMOUS_OWNER; }
>
> Again I struggle with the pre-existing `is_owner` formulation here. The
> target of the expression is a monitor and we are asking if the monitor has an
> anonymous owner.
I changed it to `has_owner_anonymous`.
> src/hotspot/share/runtime/objectMonitor.hpp line 333:
>
>> 331: bool is_stack_locker(JavaThread* current);
>> 332: BasicLock* stack_locker() const;
>> 333: void set_stack_locker(BasicLock* locker);
>
> Again `is` versus `has`, plus some general comments describing the API.
Fixed and added comments.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811600012
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811600739
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601098
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601168
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601545
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601472
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601619
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811601871
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811602000
