RFR(S): 8236035: refactor ObjectMonitor::set_owner() and _owner field setting

[Kim Barrett]

> On Jan 27, 2020, at 3:06 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
> 
> Greetings,
> 
> I'm still looking for a second reviewer on this thread. I've gone ahead
> and made changes based on David H's comments on CR0.
> 
>     JDK-8236035 refactor ObjectMonitor::set_owner() and _owner field setting
>     https://bugs.openjdk.java.net/browse/JDK-8236035
> 
> Here's the incremental webrev URL:
> 
> http://cr.openjdk.java.net/~dcubed/8236035-webrev/1-for-jdk15.inc/
> 
> Here's the full webrev URL:
> 
> http://cr.openjdk.java.net/~dcubed/8236035-webrev/1-for-jdk15.full/

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.hpp
 241   // Try to set _owner field to new_value if the current value matches
 242   // old_value. Otherwise, does not change the _owner field.
 243   void*     try_set_owner_from(void* old_value, void* new_value);

It would be nice if the comment described the result.

It would be nice if the comment described the effect on memory order.

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.inline.hpp
  88 // Clear _owner field; current value must match old_value.
  89 // If needs_fence is true, we issue a fence() after the release_store().
  90 // Otherwise, a storeload() is good enough. See the callers for more info.
  91 inline void ObjectMonitor::release_clear_owner_with_barrier(void* old_value,
  92                                                             bool needs_fence) {

Here's what I was going to say about the original version:

I would prefer the description of needs_fence to be on the API
declaration in the header.  On the other hand, I think that argument
is unnecessary specialization that has no actual effect (other than to
perhaps make the code larger and slower on some platforms).  storeload
is equivalent to fence on every platform in HotSpot.  (Either
storeload is implemented as a call to fence, or they have the same
definitions in terms of some platform-specific thing.)  I think it
should just use release_store_fence (and perhaps adjust the function's
name). 

The second version has reverted to having the callers specify the
barrier explicitly.  It looks to me like the distinction in the two
call sites is just different authors at different times.  For example,
the fence at objectMonitor.cpp:1097 is commented as being for store of
_owner vs load in unpark, so seems like it could be a storeload if one
wanted to go that way.

So I still suggest just using release_store_fence, possibly with a
function name adjustment.

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.inline.hpp
 102   void* prev = _owner;
 105   _owner = new_value;
 114   void* prev = _owner;
 119   _owner = self;

Consider using Atomic::load and Atomic::store for these, making the
intent of a relaxed atomic operation explicit.  Though that might be
seen as inconsistent with various other places where _owner is being
directly read or written.

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.inline.hpp
  90   void* prev = _owner;

Consider making this DEBUG_ONLY, and changing the reference to prev in
the later log_trace use old_value instead of prev.  This would allow
the load of _owner to be eliminated in a release build; that isn't
permitted as written, because it's volatile.

(The old_value seems to be (nearly?) always either a value we already
have for other reasons, or NULL, so the only additional cost we're
paying for it is register pressure to keep it around until the possible
tracing use.)

Similarly in the other nearby functions.

------------------------------------------------------------------------------
src/hotspot/share/runtime/objectMonitor.cpp
In ObjectMonitor::exit.
 864   if (THREAD != _owner) {
 865     void* cur = _owner;

The value of _owner is being captured in a variable to avoid multiple
reads in the code below.  I don't see any reason not to exchange these
two lines and change the compare to use cur instead of _owner.
(_owner being volatile prevents the compiler from automatically
coalescing the loads.)

Similarly in complete_exit, around lin 1124.
Similarly in check_owner, around line 1175.

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