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

[Dean Long]

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 was thinking about splitting unlock(obj) through Phi node to keep separate unlock for object coming from Interpreter

> It may be possible do something when we parse merge point but I think it is hard. What if this merge point is not at the start but somewhere later?

I think it is true that OSR nmethods only have the OSR entry point.  There is no normal entry point.  So if we did a special kind of loop unrolling, so that the OSR entry came first, we would end up with something like this, assuming OSR entry happens on the first iteration with i == 0.  The merge point/phi goes away completely, I believe.
    i = 0;
         // Trigger OSR compilation
        [ OSR entry ]
        [...]
        [montorexit on iterpreter object, with no preceding monitorenter!]
    i = 1;
            Object o = new Object();  // Never escapes
            synchronized (o) { // This monitorenter can be eliminated
                for (int j = 0; j < 100_000; ++j) {

In general we don't know which iteration will trigger OSR.  So unrolled code would look like:
    int i = OSR_start;
    [...]
    for (i = OSR_start + 1; i < 2; ++i) {

I wouldn't be surprised if generating the Unlock without the Lock breaks some assumptions elsewhere.
I'm not suggesting something like this for this PR -- just thinking it seems possible conceptually.

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

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