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

[Daniel D. Daugherty]

Hi Kim,

Thanks for the review!

Replies are embedded below...

On 1/27/20 4:44 PM, Kim Barrett wrote:
>> 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.

By "result" I'm hoping that you mean the return value... :-)


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

So here's the new comment:

   // Try to set _owner field to new_value if the current value matches
   // old_value. Otherwise, does not change the _owner field. Returns
   // the prior value of the _owner field. try_set_owner_from() provides:
   //   <fence> compare-and-exchange <membar StoreLoad|StoreStore>
   void*     try_set_owner_from(void* old_value, void* new_value);

I'm not planning to update the comment for the function definition
in objectMonitor.inline.hpp since the reader can easily see that
try_set_owner_from() is a wrapper around cmpxchg() with logging...


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

David H. has posted a long reply to this comment so I'll chime
in on this comment as a reply to that sub-thread.


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

Since these are new functions, I went ahead and updated the loads and
stores of the _owner field to Atomic::load() and Atomic::store(). I had
planned to file a cleanup bug for all of the _owner field stuff and I'll
still do that to handle the rest.


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

Done. Part of the work in my _owner field cleanup bug will be to remove
"volatile" from the _owner field once all the loads and stores use the
proper Atomic op...


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

One of reasons for the 'old_value' parameter is the "state machine"
asserts that I've added. We want to make sure that we verify the
expected old value of the _owner field before we change it in order
to detect state machine violations.


> Similarly in the other nearby functions.

Also changed set_owner_from() and set_owner_from_BasicLock().


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

Done. Since I touched the lines, I also changed the load of the
_owner field in that location to use Atomic::load().


> Similarly in complete_exit, around lin 1124.

Done; also switched to Atomic::load().


> Similarly in check_owner, around line 1175.

Done; also switched to Atomic::load().


> ------------------------------------------------------------------------------

Thanks again for the thorough review!

I've made the minor changes and I'm build testing now. I'll send out
another round of webrevs in a bit...

Dan