RFR (S): 8228857: Refactor PlatformMonitor into PlatformMutex and PlatformMonitor

[Kim Barrett]

> On Aug 7, 2019, at 10:51 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Kim,
> 
> On 8/08/2019 11:59 am, Kim Barrett wrote:
>> PlatformMutex should be noncopyable on all platforms.
> 
> I presume that is something PlatformMonitor should already be doing but is not? What is involved in making something non-copyable? (Sorry my basic C++ is pretty basic.)

Yes, PlatformMonitor ought to already be doing that.

We don't have any explicit support for declaring a class noncopyable. One
idiom that's being used is to declare the copy constructor and copy assignment
operator private and don't provide a definition for either. Then one gets a
compile time error if anything outside the class attempts to copy, and a link
time error if the copy is from inside the class (or by a friend). So in the
private part of the class

  PlatformMutex(const PlatformMutex&);
  PlatformMutex& operator=(const PlatformMutex&);

Maybe we should have a helper macro for that, but nobody has proposed adding
one. For C++11 it's

  PlatformMutex(const PlatformMutex&) = delete;
  PlatformMutex& operator=(const PlatformMutex&) = delete;

anywhere in the class (public is best, to get a reference to deleted function
error message rather than possibly an access error message).

This is a "good hygiene" thing; many of our classes "ought" to have this but
don't bother.

>> The inheritance runs the risk of destructor slicing, because the
>> PlatformMutex base class has a public non-virtual destructor. I don't
>> think we want to introduce a virtual destructor here. I don't know of
>> a solution that doesn't lose the inheritance relationship between the
>> classes. I don't know if that relationship is considered important.
> 
> Semantically a Monitor (as we define it) is a Mutex with a condition variable, so the inheritance seems quite appropriate, even if not essential.
> 
> IIUC the issue is when we have a PlatformMutex* that happens to point to a PlatformMonitor - if we delete it then we only call the PlatformMutex destructor. Why would we not just declare the destructor as virtual? What will it cost us? It seems the natural/right way to deal with inheritance in C++.

It imposes virtual function overheads (class vtable, per-object vtable
entries, &etc) on what is intended to be simple, low-overhead classes.

Do we care? One option is to ignore the problem; there's certainly precedent
for that in Hotspot! As a result, we can't enable some associated gcc warnings
(and have to actually turn some off), though as I've mentioned elsewhere some
of those warnings have other problems:
https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2019-July/026480.html

Fixing without taking on virtual function costs involves losing the
base/derived relationship between these classes. I don't know if we care about
that relationship. To do this one would introduce a new helper base class,
with each deriving separately from it.

class PlatformMutexBase ...
  ... same definition as proposed PlatformMutex, except with
      protected constructor and destructor ...
};

class PlatformMutex : public PlatformMutexBase {};
class PlatformMonitor : public PlatformMutexBase {
  ... same definition as proposed PlatformMonitor, adjusting for
      base class name change ...
};

It amounts to one more line of code, the new PlatformMutex class definition.