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

[David Holmes]

Hi Kim,

On 9/08/2019 9:17 am, Kim Barrett wrote:
>> 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&);

Okay I can add those.

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

In my limited experience it seems C++ got it backwards by providing 
these by default - copying and assignment should be opt-in.

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

Okay ...

> Do we care? One option is to ignore the problem; there's certainly precedent

Seems like the path of least resistance, and it's relatively easy to 
check that we don't assign a PlatformMonitor to a PlatformMutex field.

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

Okay so this supposes that we never declare variables of type 
PlatformMutexBase (which we shouldn't), but only PlatformMutex and 
PlatformMonitor, and those two are not assignment compatible and so we 
avoid the problem.

That's not a terrible solution, but if you don't object strongly I'll 
stick with the current arrangement.

Thanks,
David