RFR: 8322743: assert(held_monitor_count() == jni_monitor_count()) failed

[Vladimir Kozlov]

On Tue, 9 Jan 2024 22:57:27 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

> Corner case with a local (not escaped) object used for synchronization. C2 Escape Analysis thinks that it can eliminate locks for it. In most cases it is true but not in this case.
> 
> 
>         for (int i = 0; i < 2; ++i) {
>             Object o = new Object();
>             synchronized (o) { // monitorenter
>                 // Trigger OSR compilation
>                 for (int j = 0; j < 100_000; ++j) {
> 
> The test has nested loop which trigger OSR compilation. The locked object comes from Interpreter into compiled OSR code. During parsing C2 creates an other non escaped object and correctly merge both together (with Phi node) so that non escaped object is not scalar replaceable. Because it does not globally escapes EA still removes locks for it and, as result, also for merged locked object from Interpreter which is the bug.
> 
> The fix is to check that synchronized block does not have any associated escaped objects when EA decides if locks can be eliminated.
> 
> Added regression test prepared by @TobiHartmann. Tested tier1-5, xcomp and stress.
> Performance testing show no difference.

"We know from EA that the object coming from the interpreter does not escape" - we don't know what happens in Interpreter to this object. There is no information where this object is coming from (no method and no bci info). We only know that we have monitor at slot 0 which uses this object. Yes, we can do bytecode analysis to determine that but it is a lot more code.

There could be other, more complicated, ways to remove locks for this case. I was thinking about splitting  `unlock(obj)` through Phi node to keep separate `unlock` for object coming from Interpreter. Unfortunately it is not enough. We need also to keep separate synchronization blocks defined by BoxLock node. Otherwise we still eliminate all locks/unlocks during locks elimination [macro.cpp#L1946](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/macro.cpp#L1946).

Note, we can't eliminate only part of locks/unlocks associated with one synchronization block. Otherwise we can't guarantee that we have balanced locks and unlocks (we had bugs about it). So we either eliminate or keep all of them.

I think my fix is conservative solution for this issue.

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

PR Comment: https://git.openjdk.org/jdk/pull/17331#issuecomment-1885946951