RFR: 8320318: ObjectMonitor Responsible thread [v3]

[Fredrik Bredberg]

On Mon, 23 Sep 2024 12:05:01 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> src/hotspot/share/runtime/objectMonitor.cpp line 335:
>> 
>>> 333:       // ObjectMonitor::deflate_monitor() will decrement contentions
>>> 334:       // after it recognizes that the async deflation was cancelled.
>>> 335:       contention_mark.extend();
>> 
>> This is a bit scary. Previously the locking_thread would already have 1 stake in the _contentions, and recognizes that after cancelling deflation, that stake is asynchronously released by the deflation thread. This means that in practice, we have 0 stakes left in the contention counter after the CAS that swings the owner to the locking_thread succeeds. Yet the ObjectMonitorContentionMark RAII object passed in from the caller looks like it guarantees there is a stake in the _contentions throughout its scope. By explicitly adding to contentions, the locking_thread reclaimed its stake in the _contentions counter while holding the lock, guaranteeing that deflation is indeed impossible until the end of the scope.
>> The new extend() mechanism seems to consider it equivalent to not increment here and also not decrement later. +1 -1 == 0 right? However, that is not equivalent. HotSpot math works in mysterious ways. The old mechanism guaranteed the linearization point for deflation is blocked until you get out of scope. The new mechanism does not. Instead, it's up to the user to reason about for how long deflation is blocked out. It's blocked out as long as the monitor is held naturally, but if it is released and the scope is still active, there is no stake in the _contentions counter and deflation would succeed if it tries again. Things like the _waiters counter might tell the heuristics of deflation to not try again. An absence of a safepoint poll might also prevent deflation from trying again in a timely fashion. But the point is that the linearization point for deflation is no longer blocked, and the abstraction looks safer than it is.
>> 
>> In practice, I don't know of any bug because of this. Seems like deflation is in other ways blocked out in practice. But I would really prefer if extend() would add to contentions and the destructor always decrements. This way, the contract is stronger and it's easier to convince ourselves that we have not messed up. The scope would on its own prevent deflation, regardless of how it is used, which cannot be guaranteed any longer with the current extend() implementation.
>> 
>> Again, this might be better suited for a follow-up RFE.
>
> I think extends() should add to contentions in this PR since extends() is part of this PR, and initially I expected it to be sort of a refcount (with a case or two for extending the scope of the refcount).

fixed

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19454#discussion_r1775318079