RFR(L) 8153224 Monitor deflation prolong safepoints (CR7/v2.07/10-for-jdk14)

[David Holmes]

Hi Dan,

Trimming ...

On 6/11/2019 1:36 am, Daniel D. Daugherty wrote:
> 
> And my brain returns to the more fundamental question of why do we have
> OrderAccess::storeload() at L1045 and OrderAccess::fence() at L1225?
> Both sites are trying to separate the release_store(&_owner, NULL) from
> a subsequent load. In the first case:
> 
> 1044       OrderAccess::release_store(&_owner, (void*)NULL); // drop the 
> lock
> 1045       OrderAccess::storeload();                        // See if we 
> need to wake a successor
> <snip>
> 1047     if ((intptr_t(_EntryList)|intptr_t(_cxq)) == 0 || _succ != NULL) {
>
> In the second case:
> 
> 1224     OrderAccess::release_store(&_owner, (void*)NULL);
> 1225     OrderAccess::fence();                               // ST 
> _owner vs LD in unpark()
> <snip>
> 1229   Trigger->unpark();
> 
> The code has been this way for a very long time, but why?

The devil is in the detail and this is a very complex piece of code with 
very complex usage protocols. Generally speaking when you release the 
lock you want a fence() to ensure immediate visibility to other threads 
that the lock is now free. That happens in paths that will complete the 
monitor operation quickly e.g. in ExitEpilog. In other places after 
releasing the lock we may have more work to do - like wake a successor - 
so we only need the storeload() after the release_store to make sure we 
don't read the queue heads until after we've released the lock; the code 
that follows that can then have additional memory sync operations (like 
an atomic op) that implicitly achieve the effects of the fence.

> Of course, this question about the baseline code is still a sidebar.
> An interesting sidebar, but...
> 
> 
> For Async Monitor Deflation, I think we need fence() at both locations
> for proper interaction with the deflater thread.

Possibly. The devil is in the detail of the actual code paths.

>> As per previous discussion I think you still need a release_store of 
>> _owner (at least in the case where you are releasing the monitor).
> 
> I've made a mistake with this encapsulation. I made it look like a
> general setter of a new value. In reality, both callers specify
> new_value == NULL so we don't actually need the new_value parameter.
> 
> I think it needs to be something like this:
> 
>    124 // Clear _owner field; current value must match old_value.
>    125 inline void ObjectMonitor::clear_owner_from(void* old_value) {
>    126   void* prev = _owner;
>    127   ADIM_guarantee(prev == old_value, "unexpected prev owner=" 
> INTPTR_FORMAT
>    128                  ", expected=" INTPTR_FORMAT, p2i(prev), 
> p2i(old_value));
>    129   OrderAccess::release_store(&_owner, (void*)NULL);
>    130   OrderAccess::fence();
>    131   log_trace(monitorinflation, owner)("clear_owner_from(): mid=" 
> INTPTR_FORMAT
>    132                                      ", prev=" INTPTR_FORMAT, 
> p2i(this), p2i(prev));
>    133 }

When factored out like this you are forced to use the heaviest memory 
barrier needed by a given callsite and then apply it to all - ie using 
the fence() always when it might not be needed.

I'm really not a fan of this kind of factoring out in complex lock-free 
code as it hides the details of the memory sync operations. You could 
make this more obvious in the naming e.g.

inline void ObjectMonitor::release_clear_owner_and_fence(void* 
expected_owner)

or if the fence is not always needed you could parameterise the final 
memory barrier as well.

> Thanks for sticking with this part of the review.
> 
> 
>> That's it on this thread. I still have to look at version 2.08 in full.
> 
> I look forward to your feedback!

Hopefully tomorrow. Trying to track down a memory corruption issue.

Cheers,
David