On Tue, 17 Dec 2024 04:01:26 GMT, Kim Barrett <kbarr...@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
