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

[Robbin Ehn]

Hi Kim, thanks for looking at this.

On 11/19/19 4:55 PM, Kim Barrett wrote:
> ------------------------------------------------------------------------------
> 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.

Thanks, removed.

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

Changed name.

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

No future plans, no.
Are you suggesting doing a synchronous safepoint here instead?

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

Added back.

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

(sending v3 after I fixed David's comments)

Thanks, Robbin