RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning

[Coleen Phillimore]

On Wed, 23 Oct 2024 06:15:27 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Why do we need to cache it? Is it the implicit barriers related to accessing the threadObj oop each time?

We cache threadObj.thread_id in JavaThread::_lock_id so that the fast path c2_MacroAssembler code has one less load and code to find the offset of java.lang.Thread.threadId in the code.  Also, yes, we were worried about performance of the barrier in this path.

> src/hotspot/share/runtime/objectMonitor.hpp line 174:
> 
>> 172: 
>> 173:   int64_t volatile _owner;  // Either tid of owner, NO_OWNER, ANONYMOUS_OWNER or DEFLATER_MARKER.
>> 174:   volatile uint64_t _previous_owner_tid;  // thread id of the previous owner of the monitor
> 
> Looks odd to have the current owner as `int64_t` but we save the previous owner as `uint64_t`. ??

I was wondering what this was too but the _previous_owner_tid is the os thread id, not the Java thread id.


$ grep -r JFR_THREAD_ID
jfr/support/jfrThreadId.hpp:#define JFR_THREAD_ID(thread) (JfrThreadLocal::external_thread_id(thread))
jfr/support/jfrThreadId.hpp:#define JFR_THREAD_ID(thread) ((traceid)(thread)->osthread()->thread_id())
runtime/objectMonitor.cpp:    _previous_owner_tid = JFR_THREAD_ID(current);
runtime/objectMonitor.cpp:    iterator->_notifier_tid = JFR_THREAD_ID(current);
runtime/vmThread.cpp:  event->set_caller(JFR_THREAD_ID(op->calling_thread()));

> src/hotspot/share/runtime/synchronizer.cpp line 1440:
> 
>> 1438: }
>> 1439: 
>> 1440: ObjectMonitor* ObjectSynchronizer::inflate_impl(JavaThread* inflating_thread, oop object, const InflateCause cause) {
> 
> `inflating_thread` doesn't sound right as it is always the current thread that is doing the inflating. The passed in thread may be a different thread trying to acquire the monitor ... perhaps `contending_thread`?

If it's always the current thread, then it should be called 'current' imo.

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

PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2433252605
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816550112
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816551794