RFR(s): 8234086: VM operation can be simplified
Robbin Ehn
robbin.ehn at oracle.com
Wed Nov 20 09:38:50 UTC 2019
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
More information about the hotspot-runtime-dev
mailing list