RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v3]

[Patricio Chilano Mateo]

On Tue, 22 Oct 2024 06:31:47 GMT, David Holmes <dholmes at 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