RFR: 8225631: Consider replacing muxAcquire/Release with PlatformMonitor

[Coleen Phillimore]

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?

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?

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.

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

Changes requested by coleenp (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/1196