RFR(s): 8234086: VM operation can be simplified

[Kim Barrett]

> On Nov 19, 2019, at 8:51 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
> 
> Hi David, thanks for having a look.
> 
> On 11/19/19 2:27 PM, David Holmes wrote:
>> Hi Robbin,
>> On 19/11/2019 9:05 pm, Robbin Ehn wrote:
>>> Hi all, please review.
>> Preliminary comments ... so ...
>> +class VM_Operation : public StackObj {
>>   public:
>>    enum Mode {
>>      _safepoint,       // blocking,        safepoint, vm_op C-heap allocated
>> -    _no_safepoint,    // blocking,     no safepoint, vm_op C-Heap allocated
>> -    _concurrent,      // non-blocking, no safepoint, vm_op C-Heap allocated
>> -    _async_safepoint  // non-blocking,    safepoint, vm_op C-Heap allocated
>> +    _no_safepoint    // blocking,     no safepoint, vm_op C-Heap allocated
>>    };
>> You are basically getting rid of concurrent and async_safepoint VM op capability. Okay. But you're also making all VM ops StackObj so all those "VM op C-heap allocated" comments are no longer correct. Also many of the comments around the VM ops you have changed from async to sync,and from C-heap to stackobj, are also no longer correct.
>> Please update.
> 
> Not sure if I found all:
> http://cr.openjdk.java.net/~rehn/8234086/v2/inc/webrev/index.html
> http://cr.openjdk.java.net/~rehn/8234086/v2/full/webrev/index.html

------------------------------------------------------------------------------
src/hotspot/share/runtime/biasedLocking.cpp
  83     // Use async VM operation to avoid blocking the Watcher thread.
  84     // VM Thread will free C heap storage.

These comments appear to be obsolete.

------------------------------------------------------------------------------
src/hotspot/share/runtime/safepoint.cpp
 535 bool SafepointSynchronize::is_forced_cleanup_needed() {
 536   return ObjectSynchronizer::force_monitor_scavage();
 537 }

src/hotspot/share/runtime/synchronizer.cpp 
 917   return force_monitor_scavage();

src/hotspot/share/runtime/synchronizer.cpp 
 920 bool ObjectSynchronizer::force_monitor_scavage() {

force_monitor_scavage typo in "scavage".

And that name seems like it has side effects (forces something), not a
predicate. It's testing whether ForceMonitorScavenge is true, but I
think a better name is needed.

------------------------------------------------------------------------------
src/hotspot/share/runtime/synchronizer.cpp
1006   if (Atomic::load(&ForceMonitorScavenge) == 0 && Atomic::xchg (1, &ForceMonitorScavenge) == 0) {
1007     VMThread::check_cleanup();

I was going to make a couple nit-pick comments here, but this seems to
be soon to be dead code (only reached when deprecated MonitorBound has
a non-default value).

Not sure it's worth changing this and adding check_cleanup just for
use here though, given this code is going away soon. Are there planned
future uses of check_cleanup?

------------------------------------------------------------------------------
src/hotspot/share/runtime/vmOperations.hpp  
Removed:
 156   virtual ~VM_Operation() {}

Simple removal may cause warnings with some flags that have been
discussed being enabled. This change violates the "usual rule" of
public and virtual or non-public for base class destructors. Of
course, that rule (and any warnings that might try to check for it)
doesn't account for non-heap-allocatable classes. And HotSpot is full
of violations of that rule, so we're probably a long way from being
able to turn on some of those kinds of warnings.

OTOH, leaving the destructor alone has very little cost.

------------------------------------------------------------------------------
src/hotspot/share/runtime/vmOperations.hpp 
 190   bool evaluate_at_safepoint() const {

Nice to see this no longer being virtual, and the preceeding big block
caution comment being gone.

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