RFR: 8225631: Consider replacing muxAcquire/Release with PlatformMonitor

[David Holmes]

Hi Coleen,

Thanks for reviewing this.

On 13/11/2020 11:51 pm, Coleen Phillimore wrote:
> On Fri, 13 Nov 2020 00:51:02 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>> This RFE was filed a while ago to see if we could get rid of `muxAcquire/release` to reduce overall code complexity. Initially some usages seemed to suffer some slight performance loss when I tried this. However, since then all but one usage have been removed and the final case, the `gInflationLock`'s in `ObjectSynchronizer read_stable_mark` does not suffer any performance degradation. It can also use `PlatformMutex` rather than `PlatformMonitor`.
>>
>> On the plus side we remove a large chunk of complex synchronization code, we delete a `ParkEvent` per thread, and an int field per `ParkEvent` - which is good for footprint.
>>
>> Testing:
>> - GH actions
>> - mach5 tiers 1-3
>> - selected performance testing (guided by that used for JDK-8253064)
>>
>> Thanks,
>> David
> 
> I've been waiting so long for this change!  I love it.
> 
> src/hotspot/share/runtime/synchronizer.cpp line 807:
> 
>> 805:         ObjectSynchronizer::gInflationLocks[ix]->lock();
>> 806:         while (obj->mark() == markWord::INFLATING()) {
>> 807:           // Beware: naked_yield() is advisory and has almost no effect on some platforms
> 
> Maybe you could hoist Thread::current() out of this loop?

I could but there's no real motivation for doing so as we are not trying 
to minimise the execution cost in this code. On the contrary, by 
lengthening the execution time of the inner chunk of code we probably 
improve the chances that the concurrent inflation is over. I will assume 
Dave Dice had his reasons for the current format.

> src/hotspot/share/runtime/synchronizer.hpp line 119:
> 
>> 117:
>> 118:   static const int NINFLATIONLOCKS = 256;
>> 119:   static os::PlatformMutex* gInflationLocks[NINFLATIONLOCKS];
> 
> Why export these?  Why not keep them local to synchronizer.cpp?

Good question! :) I've had this patch sitting around for 14 months and 
have no recollection as to why I may have exported these ... perhaps a 
misconception that they needed to be part of the class so that the 
initialize() method could access them.

Fixed.

> src/hotspot/share/runtime/synchronizer.cpp line 801:
> 
>> 799:         // would detach the list and set the markword to inflated with a single CAS and
>> 800:         // then for each thread on the list, set the flag and unpark() the thread.
>> 801:         int ix = (cast_from_oop<intptr_t>(obj) >> 5) & (ObjectSynchronizer::NINFLATIONLOCKS-1);
> 
> I was trying to understand what this did.  The comment is a lot less helpful than the code, but there are 3 TODO comments above this that I don't think we'll ever TODO (or haven't in the past 20 years) and refers to SafepointSynchronize::do_call_back().  Can you remove these in this change since adjacent?  Also these ideas for future work in this comment really don't look like good ideas to me.

Line 801 simply uses part of the object address to index into the lock 
array - I've added a comment to that effect.

I was going to leave the TODO's as Dice's code is always littered with 
these to provide guidance on future potential enhancements. But in this 
case the stale reference to do_call_back needed to be fixed, and as you 
note we're unlikely to ever actually make any changes directly to this 
part of the code, so I did remove them.

Thanks,
David
-----

> -------------
> 
> Changes requested by coleenp (Reviewer).
> 
> PR: https://git.openjdk.java.net/jdk/pull/1196
>