RFR: 8322743: assert(held_monitor_count() == jni_monitor_count()) failed [v2]

[Emanuel Peter]

On Wed, 17 Jan 2024 20:20:05 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.
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Address review comments

I'll need to study EA from the ground up to really review this. So for now just a couple of questions:

> During parsing C2 creates an other non escaped object and correctly merge both together (with Phi node)

Why do we create this non-escaped object during parsing? Is that the one that comes in through the OSR slot?

What exactly makes this case different to another case where we just have a lock come in through the OSR slot, but no Phi that merges them with an object allocation inside the OSR body?

src/hotspot/share/opto/escape.cpp line 2888:

> 2886:  *
> 2887:  * Return true if lock/unlock can be eliminated.
> 2888:  */

Suggestion:

// The lock/unlock is unnecessary if we are locking a non-escaped object,
// unless synchronized block (defined by BoxLock node) has other escaped objects
// (for example, locked object come from Interpreter in OSR compilation).
//
// Return true if lock/unlock can be eliminated.

This would be the first use in this file of multi-line comment :man_shrugging:

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17331#pullrequestreview-1830123844
PR Review Comment: https://git.openjdk.org/jdk/pull/17331#discussion_r1457709717