RFR: 8255678: Add Mutex::try_lock version without rank checks [v2]

[David Holmes]

On Wed, 18 Nov 2020 03:10:21 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> src/hotspot/share/runtime/mutex.cpp line 161:
>> 
>>> 159:   if (_owner == self) {
>>> 160:     return false;
>>> 161:   }
>> 
>> This is being fixed by https://github.com/openjdk/jdk/pull/1247
>
> I'll wait until you push and then I'll merge. I still have to check the owner here though, because if the thread already owns the lock, calling check_rank() from try_lock will do the wrong check (it will assume it's being called from wait()). An alternative would be to add a boolean parameter to check_rank(), set only from try_lock() so I can differentiate the two calls.

I think a different refactoring will be needed here so that:
try_lock() -> try_lock_inner(true);
try_lock_without_rank_check() -> try_lock_inner(false);
try_lock_inner(bool check_rank) { ... }
That way code duplication is minimised.

>> src/hotspot/share/runtime/mutex.cpp line 220:
>> 
>>> 218:   JavaThread* const self = JavaThread::current();
>>> 219:   // Safepoint checking logically implies an active JavaThread.
>>> 220:   guarantee(self->is_active_Java_thread(), "invariant");
>> 
>> Not clear why this is necessary here, and why it is a guarantee rather than assert ??
>
> I looked at the history of this check. The guarantee has been since day1 on the hg repo as "self->is_Java_thread()" and then changed in 8226228 to "self->is_active_Java_thread()". I moved it at the beginning of wait() since it seems the right place to check it just after retrieving self. It doesn't make sense to keep it as a guarantee when every other check is an assert so I will change it.

Sorry I missed that this had simply been moved.

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

PR: https://git.openjdk.java.net/jdk/pull/1242