On Wed, 23 Oct 2024 00:35:19 GMT, Patricio Chilano Mateo <pchilanom...@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
