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

[Dean Long]

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.

I was thinking that the OSR situation is similar to this:

        for (int i = 0; i < 2; ++i) {
            Object o = osr ? static_volatile_field /* black hole, can't eliminate */ : new Object() /* can eliminate */;
            synchronized (o) { // monitorenter
                // Trigger OSR compilation
                for (int j = 0; j < 100_000; ++j) {

but maybe we can do better.  If C2 can eliminate allocations/locks for non-escaping objects, and that works in one direction C2 --> interpreter (deopt), then the reverse direction, interpreter --> C2 (OSR) might also be made to work.  In other words, I think we could eliminate the lock, even in the OSR case.  We know from EA that the object coming from the interpreter does not escape, so if load_interpreter_state did the reverse of deopt, we would end up with a scalar-replaced object.  Deopt does scalar-replaced object --> materialized, so OSR would need to do materialized --> scalar-replaced object.  The fields of the scalar-replaced object would be populated from the fields of the interpreter object, but ignoring fields with a default (0) value.  Assuming I'm right, and this could work, that doesn't mean it's worth doing.  I'm just throwing this idea out mostly for completeness.

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

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