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

[Emanuel Peter]

On Fri, 9 Feb 2024 19:25:20 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:
> 
>   Renaming

The approach looks better now :)
Though it would be good if someone who has a deeper understanding of the locking code would have a look as well.

src/hotspot/share/opto/compile.cpp line 4955:

> 4953: // if locks coarsening optimization removed lock/unlock from them.
> 4954: // Such regions become unbalanced and we can't execute other
> 4955: // locks elimination optimization on them.

The comment suggests you mark boxes as "coarsened" if ...
But really you mark them as "unbalanced", right? Because you have the condition that the locks have to already be "coarsened".

src/hotspot/share/opto/compile.cpp line 4964:

> 4962:       AbstractLockNode* alock = locks_list->at(0)->as_AbstractLock();
> 4963:       BoxLockNode* box = alock->box_node()->as_BoxLock();
> 4964:       if (alock->is_coarsened() && !box->is_unbalanced()) { // Not marked already

Is it possible that the position 0 `box` is already marked as unbalanced, but there are other `this_box` for positions `j=1..` that are not yet marked but should be?
Is it really ok that we are noch cheching all-with-all but only first-with-all?

src/hotspot/share/opto/compile.cpp line 4979:

> 4977:   }
> 4978: }
> 4979: 

A code-style suggestion:
You use lots of nesting here, with deeper and deeper if blocks. But since you don't have any else-blocks, you could easily flatten your code by using `if (condition) { continue; }`, what I call "bailout instead of nesting". But that may be up to personal taste ;)

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

PR Review: https://git.openjdk.org/jdk/pull/17697#pullrequestreview-1878441924
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488207160
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488217470
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488227458