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

[David Holmes]

Hi Kim,

Picking up on one area of comments ...

On 28/01/2020 7:44 am, Kim Barrett wrote:
>> On Jan 27, 2020, at 3:06 PM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
> ------------------------------------------------------------------------------
> 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 code should use the operation that is semantically required for 
correctness, irrespective of what the underlying implementation is 
equivalent to.

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

Actually this is all Dave Dice code from 2005.

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

Must admit I find that comment hard to understand.

Let's examine the two code fragments. Here's current code for first chunk:

  919     Atomic::release_store(&_owner, (void*)NULL);   // drop the lock
  920     OrderAccess::storeload();                      // See if we 
need to wake a successor
  921     if ((intptr_t(_EntryList)|intptr_t(_cxq)) == 0 || _succ != NULL) {
  922       return;
  923     }

Back in 2005 we had some additional commentary that explained this:

  // Observe the Dekker/Lamport duality:
2801       // A thread in ::exit() executes:
2802       //   ST Owner=null; MEMBAR; LD EntryList|cxq.
2803       // A thread in the contended ::enter() path executes the 
complementary:
2804       //   ST EntryList|cxq = nonnull; MEMBAR; LD Owner.

The MEMBAR referenced at L2802 is the storeload we see above in current 
code. The enter code is covered by this comment (in part) in current code:

  527   // Note the Dekker/Lamport duality: ST cxq; MEMBAR; LD Owner.
  528   // In this case the ST-MEMBAR is accomplished with CAS().

So the storeload is all that is required here.

In ExitEpilog we currently have:

1094   // Drop the lock
1095   Atomic::release_store(&_owner, (void*)NULL);
1096   OrderAccess::fence();                               // ST _owner 
vs LD in unpark()
1097
1098   DTRACE_MONITOR_PROBE(contended__exit, this, object(), Self);
1099   Trigger->unpark();

Back in 2005 this was:

2670    _owner = NULL ;
2671    OrderAccess::fence() ;
2672
<unrelated comments elided>
2682    if (SafepointSynchronize::do_call_back()) {
2683       TEVENT (unpark before SAFEPOINT) ;
2684    }
2685
<unrelated comments elided>
2699    Trigger->unpark() ;

No explanation for the fence() - though to me this is needed to ensure 
visibility of the store to _owner, as well as ensuring ordering with the 
unpark code. The comment was added (again by Dice) in Feb 2007. So the 
question is:
- is the fence() stronger than what we need, or is the comment incomplete?
I tend to favor the latter, given the fence has always been there.

So I remain in favor of isolating the trailing memory-barrier from the 
release_store of _owner.

Cheers,
David
-----


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