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

[Kim Barrett]

On Tue, 17 Dec 2024 00:23:01 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> src/hotspot/share/nmt/memTracker.hpp line 281:
>> 
>>> 279:   // Same as MutexLocker but can be used during VM init while single threaded and before mutexes are ready or current thread has been assigned.
>>> 280:   // Performs no action during VM init.
>>> 281:   class NmtVirtualMemoryLocker: public ConditionalMutexLocker {
>> 
>> Should not derive from ConditionalMutexLocker - it's not designed to be a base class.
>> Either use has-a relationship, or just derive directly from MutexLockerImpl, since we don't need
>> the assert in ConditionalMutexLocker anyway.  And I'm surprised there isn't a constructor taking
>> the current thread, like all the other locker variants.
>
> 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.)

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

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