[jdk17] RFR: 8268347: C2: nested locks optimization may create unbalanced monitor enter/exit code

[Vladimir Kozlov]

On Mon, 14 Jun 2021 19:11:57 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> There are several restrictions when nested locks should be optimized. One of them is nested locks optimization (NLO) requires one BoxLock node per one lock/unlock region. Unfortunately loop unswitching optimization can create such case because loop cloning does not clone BoxLock nodes which are outside loops (they are pinned to Root node). Note, Lock/Unlock nodes are subclass of Call node - all loop optimizations are skipped, except unswitching. 
>> 
>> NLO happens during Macro nodes elimination. But before that Lock/Unlock coarsening optimization happens during IGVN as early as right after parsing - even before Escape analysis.
>> 
>> Failing case: 
>> 
>> while (a) { 
>>     synchronized(obj) { // OLR (outer lock region) 
>>           if (loop_invar_check) { // class check for biomorphic call in the test case
>>               synchronized(obj) { // LR1 } // (lock region 1) 
>>           } else { 
>>              // no synchronization 
>>           } 
>>           synchronized(obj) { // LR2 } // (lock region 2) 
>>     } 
>> } 
>> 
>> And after loop unswitching: 
>> 
>>  if (loop_invar_check) { 
>>     while (a) { 
>>         synchronized(obj) { // OLR (outer lock region) 
>>              synchronized(obj) { // LR1 } // (lock region 1) 
>>              synchronized(obj) { // LR2 } // (lock region 2) 
>>         } 
>>     } 
>> } else { 
>>     while (a) { 
>>         synchronized(obj) { // COLR (Clone of outer lock region) 
>>              synchronized(obj) { // CLR2 } // (Clone of lock region 2) 
>>         } 
>>     } 
>> } 
>> 
>> After loop unswitching for the code in first branch Lock/Unlock coarsening optimization marked Unlock node from LR1 and Lock from LR2 as "Coarsened". 
>> 
>> When NLO (nested lock optimization) started, it overwrite state for Unlock node from LR1 as "Nested" because it passed all conditions: it belong to simple region with one BoxLock node and inside outer OLR lock region. 
>> But NLO did not overwrite state of Lock node from LR2 because this region shares BoxLock node with its clone CLR2 in second branch of unswitched loop. As result this Lock node stays "Coarsened". Later, when locks are eliminated, Lock/Unlock nodes in LR1 are eliminated as "Nested". And Lock node in LR2 is eliminated as "Coarsened" leaving behind un-matching Unlock node from LR2. 
>> 
>> The solution I propose is to catch the case when coarsened lock and unlocks are unbalanced and recompile method without "Coarsening locks" optimization. For that each group of coarsened locks/unlocks is recorded in Compile's growable array in `add_coarsened_locks()` and before lock elimination in `macro.cpp` consistency locks/unlocks in recorded groups is verified in `coarsened_locks_consistent ()`:
>>  - all stay `coarsened`
>>  - all were changed to other king (`nested` or `noescobj`)
>>  - all were removed during IR graph transformations when other Macro nodes are removed
>> 
>> I also added more debugging information for case when nested lock optimization checks are failing.
>> And more information about locks elimination in `LogCompilation` output.
>> 
>> Added regression test.
>> Tested tier1-9.
>
> test/hotspot/jtreg/compiler/locks/TestNestedLocksElimination.java line 70:
> 
>> 68:     }
>> 69: 
>> 70:     TestNestedLocksElimination getHolder(TestNestedLocksElimination s1, TestNestedLocksElimination s2, int count) {
> 
> Should have this comment here also:
> 
>     `// Don't inline`

Will do.

> test/hotspot/jtreg/compiler/locks/TestNestedLocksElimination.java line 113:
> 
>> 111: 
>> 112:         char[] c = new char[100];
>> 113:         // warmup
> 
> Usually when I see a `warmup` comment, there's a second loop
> that does the real run. In this case, I see the `warmup` comment
> and then the end of the test and I wonder if the real run is missing.

Will remove. Left over from test development.

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

PR: https://git.openjdk.java.net/jdk17/pull/38