RFR: 8346123: [REDO] NMT should not use ThreadCritical

[David Holmes]

On Tue, 17 Dec 2024 04:01:26 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> You should be able to use a ConditionalMutexLocker directly to handle this situation - it is one of the usecases that caused CML to be introduced.
>
> CML could perhaps be used has-a (though that might be messy), but should not
> be used is-a.  One of the basic rules for base classes is to have either a
> public virtual destructor (don't do that here) or a non-public non-virtual
> destructor, to prevent accidental slicing.  MutexLockerImpl has that - it was
> designed to be a base class.  CML doesn't - it was not designed to be a base
> class.  (There is a common (though not universal) opinion that classes should
> either be base or concrete classes, not both.) I know we violate this rule all
> over the place; that doesn't mean we should do so here.
> 
> But really, there's no benefit to using CML here. Deriving from
> MutexLockerImpl is about the same number of characters, and seems clearer to
> me because it's more direct.
> 
> 
> class NmtVirtualMemoryLocker : public MutexLockerImpl {
> public:
>   NmtVirtualMemoryLocker()
>     : MutexLockerImpl(_done_bootstrap ? NmtVirtualMemory_lock : nullptr,
>                       Mutex::_no_safepoint_check_flag)
>   {}
> }
> 
> 
> Related, I just noticed that MutexLockerImpl is copyable, but shouldn't be.
> (I remember a discussion a long time ago about whether StackObj should be
> noncopyable, which foundered on a combination of some significant number of
> existing uses that violated that restriction, along with disagreement about
> the meaning of deriving from StackObj.  So we're not getting noncopyable from
> there.)

`MutexLockerImpl` was not intended for external subclassing, but as the internal base class for `MutexLocker`, and `ConditionalMutexLocker`.  CML was provided for exactly this kind of situation: conditionally locking the mutex.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22745#discussion_r1887992575