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

[Robert Toyonaga]

On Tue, 17 Dec 2024 06:57:59 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> 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.

Ok I see. I will stop using `ConditionalMutexLocker` as a base class. Instead, I'll just get rid of `NmtVirtualMemoryLocker` and use `ConditionalMutexLocker` directly.

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

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