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

[David Holmes]

On Wed, 23 Oct 2024 00:35:19 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> 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.

I like the idea of using `owner_id_from` but it then suggests to me that `JavaThread::_lock_id` should be something like `JavaThread::_monitor_owner_id`. 

The use of `tid` in comments can be confusing when applied to a `JavaThread` as the "tid" there would normally be a reference of its `osThread()->thread_id()" not it's `threadObj()->thread_id()`. I don't have an obviously better suggestion though.

>> 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?

old_tid/new_tid works for me.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811933408
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811935087