RFR (S) 8074355: make MutexLocker smarter about non-JavaThreads

[David Holmes]

Hi Coleen,

On 27/04/2019 2:10 am, coleen.phillimore at oracle.com wrote:
> Summary: Use safepoint_check_always/safepoint_check_never instead of 
> safepoint_check_sometimes for locks that are taken by JavaThreads and 
> non-JavaThreads

To clarify: the safepoint_check_[always|never|sometimes] pertains only 
to the behaviour of JavaThreads that use the lock, independently of 
whether a lock may be used by both JavaThreads and non-JavaThreads.

> This patch moves all but 3 of the locks to not be 
> safepoint_check_sometimes.  We have plans to fix these three.  Also, 

So have you established that the reasons these were 'sometimes' locks no 
longer apply and so it is correct to change them? Or are you relying on 
testing to expose any mistaken assumptions?

> this patch allows for being explicit about safepoint checking or not 
> when the thread is a non-java thread, which is something that Kim 
> objected to in my first patch.

I don't understand what you mean by this. NJTs can currently call either 
lock() or lock_without_safepoint_check().

> Tested with mach5 tier1-3.
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8074355.03/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8074355

src/hotspot/share/gc/shared/oopStorage.cpp

How can these mutexes go from currently always needing safepoint checks 
to now never needing them? Are they in fact never used by JavaThreads?

---

src/hotspot/share/runtime/mutex.hpp

  95   void check_safepoint_state   (Thread* self, bool safepoint_check) 
   NOT_DEBUG_RETURN;

Please use the same parameter name as in the implementation: 
do_safepoint_check.

109   // Java and NonJavaThreads. When the lock is initialized with 
_safepoint_check_always,
  110   // that means that whenever the lock is acquired by a 
JavaThread, it will verify that each
  111   // acquition from a JavaThread is done with a safepoint check.

That can simplify to just:

109   // Java and NonJavaThreads. When the lock is initialized with 
_safepoint_check_always,
110   // that means that whenever the lock is acquired by a JavaThread, 
it will verify that
111   // it is done with a safepoint check.

Should we then continue:

111   // it is done with a safepoint check. In corollary when the lock
112   // is initialized with _safepoint_check_never, that means that
113   // whenever the lock is acquired by a JavaThread it will verify
114   // that it is done without a safepoint check.

?

---

38   SafepointCheckRequired not_allowed = do_safepoint_check ? 
_safepoint_check_never : _safepoint_check_always;
39   assert(!self->is_Java_thread() || _safepoint_check_required != 
not_allowed,

I found this code very difficult to understand due to some previous 
choices. All of the names that start with underscore give the illusion 
(to me) of being variables (or at least being the same kind of thing) 
but two are enum values and one is a field. Using 
this->_safepoint_check_required would at least make it clearer which is 
the field.

  43   // Allow NonJavaThreads to lock and wait with a safepoint check 
for locks that may be shared with JavaThreads.
  44   assert(self->is_Java_thread() || !do_safepoint_check || 
_safepoint_check_required != _safepoint_check_never,
  45          "NonJavaThreads can only check for safepoints if shared 
with JavaThreads");

This is very confusing: NJTs don't do safepoint checks. I think what you 
mean here is that you will allow a NJT to call lock() rather than 
lock_without_safepoint_check() but only if the mutex is "shared with 
JavaThreads". But always/sometimes/never has nothing to with whether the 
lock is shared between JTs and NJTs. I understand that a NJT-only mutex 
should, by convention, be created with _safepoint_check_never - but it 
really makes no practical difference. Further, a mutex used only by 
JavaThreads could in theory also be _safepoint_check_never.

47   // Only Threads_lock, Heap_lock and SR_lock may be 
safepoint_check_sometimes.
48   assert(_safepoint_check_required != _safepoint_check_sometimes || 
this == Threads_lock || this == Heap_lock ||
49          this->rank() == suspend_resume,
50          "Lock has _safepoint_check_sometimes %s", this->name());

This assert belongs in the constructor, not in every lock operation, as 
it depends only on the monitor instance not on the thread doing the lock.

---

Thanks,
David


> Thanks,
> Coleen