RFR: 8319048: Monitor deflation unlink phase prolongs time to safepoint [v3]

[Aleksey Shipilev]

On Wed, 8 Nov 2023 12:23:12 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> src/hotspot/share/runtime/synchronizer.cpp line 152:
>> 
>>> 150:           break;
>>> 151:         }
>>> 152:         if (prev == nullptr && Atomic::load(&_head) != m) {
>> 
>> If the only concurrent activity is list inserts that we are protecting against when processing the first batch, why not just let head stay intact, and commence the batch at the second element in the list (i.e. at head->next_om()) and save some of this effort to bound the walk for prev as prev (i.e. head) won't change while we walk the first (or any) batch. I realize then that one might be left with one element in the list. May be that's not a big deal especially if there are lots of inserts happening (so it won't remain at the head). When inserts are never happening, may be this isn't a big deal? (Not sure.)
>
> Oh, this is an interesting idea, it did not occur to me. Let me see if we can make it work. I think we can even go back to the `head` and check if `head` can be deflated in one last shot. The benefit of doing all this would be guaranteeing that we only update the `head` once, regardless of how many batches we do, and we would not have additional branch in the gathering loop.

This does not work, unfortunately.

There is a hidden invariant: we need to unlink *all* deflated monitors. Otherwise, we would see deflated marker in downstream code that uses the monitor lists, which would break the protocol. (There are asserts that check this.) So if we bypass the first OM _and it turns out to be deflated_, we need to deflate that OM too. But inserts can happen meanwhile, and we would lose that initial OM somewhere in the middle of the list, which means we would need to go and search for it, at very least because we would need to figure out its prev.

Took me a while to understand this. Current code avoids this by carefully tracking `head` updates and searching for new `prev` directly. I will add more verification code in this method to capture the invariant.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16412#discussion_r1388322480