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

[Vladimir Kozlov]

On Tue, 13 Feb 2024 16:28:18 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Renaming
>
>> The fix is to add check for unbalanced synchronized region (locks or unlocks) are missing.
> 
> You should probably update the PR description now.

@eme64 I updated changes based on your comments.

> 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".

I updated comment

> 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?

You are right. I removed `!box->is_unbalanced()` check.

> 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 ;)

I like nesting - it clear shows conditions under which nested code is executed. And it has less lines.
I don't think this code has deep nesting. If it was more than 10 nesting I would agree to your suggestion.

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

PR Comment: https://git.openjdk.org/jdk/pull/17697#issuecomment-1942258366
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488481453
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488481805
PR Review Comment: https://git.openjdk.org/jdk/pull/17697#discussion_r1488478923