RFR: 8324969: assert(false) failed: Non-balanced monitor enter/exit! [v4]

[Dean Long]

On Tue, 13 Feb 2024 19:38:12 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> The issue is that when we do nested locks elimination we don't check that synchronized region has both `Lock` and `Unlock` nodes.
>> Which can happen if locks coarsening optimization eliminated pair of Unlock/Lock nodes from adjacent locking regions before we check for nested locks.
>> 
>> Consider this code (all locks/unlocks use the same object):
>> 
>>     lock1(obj) { // outer synchronized region
>>          lock2(obj) {
>>              // nested synchronized region 1
>>          } unlock2(obj);
>>          lock3(obj) {
>>              // nested synchronized region 2
>>          } unlock3(obj);
>>     } unlock1(obj);
>> 
>> If `lock3` directly follows `unlock2` (no branches or safepoints) locks coarsening optimization will remove them:
>> 
>> 
>>     lock1(obj) { // outer synchronized region
>>          lock2(obj) {
>>              // nested synchronized region 1
>>          };
>>          {
>>              // nested synchronized region 2
>>          } unlock3(obj);
>>     } unlock1(obj);
>> 
>> 
>> Nested locks elimination code checks only `Lock` node in one region to find if it is nested (inside other lock region which use the same object) and then eliminate it. So we end up with not eliminated `Unlock` node in second nested region.
>> 
>> Why we don't hit this issue before? Normally nested locks elimination executed first and only then we do locks coarsening elimination. In the example above we eliminate all nested `Lock` and `Unlock` nodes, leaving only outer `Lock` and `Unlock`.
>> 
>> The additional factors which leads to the failure is fully unrolled loop around nested sync regions and some allocation to trigger Escape Analysis:
>> 
>> 
>>     lock1(obj) { // outer synchronized region
>>          Test var = new Test(); // Triggers EA
>>          for (I = 0; I < 3; i++) { // small iteration number to fully unroll
>>              lock2(obj) {
>>                  // nested synchronized region 1
>>              } unlock2(obj);
>>              lock3(obj) {
>>                  // nested synchronized region 2
>>              } unlock3(obj);
>>          }
>>     } unlock1(obj);
>> 
>> Before executing Escape Analysis we do loops optimization to simplify graph: [compile.cpp#L2332](https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/compile.cpp#L2332)
>> We also allow to fully unroll short loops (LoopOptsMaxUnroll) to remove merges from graph. It helps EA eliminate allocations.
>> Such unrolling creates several `Lock` and `Unlock` nodes per synchronized region. But nested locks eliminat...
>
> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   More comments. Adress review.

> Nested locks elimination code checks only Lock node in one region to find if it is nested (inside other lock region which use the same object) and then eliminate it. So we end up with not eliminated Unlock node in second nested region.

If lock coarsening just marked the Lock node as coarsened, but did not remove it, then Nested locks elimination would still find the Lock node and work correctly, right?  Can't we just remove coarsened locks at a later time, after locks elimination?

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

PR Comment: https://git.openjdk.org/jdk/pull/17697#issuecomment-1942830883