On Tue, 17 Dec 2024 00:23:01 GMT, David Holmes <dhol...@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