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

[Emanuel Peter]

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

Ok, so I did some rudimentary study of EA. And now this PR makes much more sense ;)

Let me summarize my understanding of the issue:
An object gets allocated in interpreter, and we lock on it in the interpreter.
OSR is triggered, the object is passed in as OSR parameter, we hold the lock.
The OSR control flow now looks like this:


StartOSR:
  LoadP -> load the object created in interpreter
  we have not_global_escape(LoadP) == false
  so this is correctly marked as escaping
  now the osr path injects into the middle of the loop

Loop:
  Phi -> merge interpreter obj and that from this compiled code
  we have not_global_escape(Phi) == false
  ...
  Unlock(Phi)
  ...
  check some condition, maybe return
  ...
  obj = CheckCastPP( Allocate(i.e. new Object()) )
  we have not_global_escape(obj) == true
  this is correct, the object will never escape
  Lock(obj)
  ...
  goto Loop


So if I understand this correctly, the marking in/with the ConnectionGrap is correct:
- The object passed in through OSR is marked as escaping.
- The object created locally is marked as non-escaping.
- The loop-phi that merges the two must therefore also be possibly escaping.

The question is then with the condition of Lock removal:
Can we remove the lock, just because its object is marked as non-escaping?
At first glance: obviously, because nobody else could ever have the object, and so nobody can ever lock/unlock it.

In the example, if we look at the Unlock node, we cannot remove it (at least at first):
its object is possibly escaping, because the Phi is not marked non-escaping.
But we can remove the Lock, since its object is non-escaping.
This is where the trouble starts.

I think it is exactly for this reason, that @vnkozlov thinks one cannot just look at the object of the individual Lock/Unlock node, but one has to look at all Lock/Unlock nodes of a BoxLock, and see if all objects are non-escaping.

@vnkozlov please correct me if I got something wrong ;)

src/hotspot/share/opto/callnode.cpp line 2004:

> 2002:     //
> 2003:     ConnectionGraph *cgr = phase->C->congraph();
> 2004:     if (cgr != nullptr && cgr->can_eliminate_lock(this)) {

I guess if you make this change, then you probably would also want to rename `NonEscObj` and `set_non_esc_obj` and `is_non_esc_obj`, right? Now it is not just about being non-escaped, but the more complex semantics of `can_eliminate_lock`.

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17331#pullrequestreview-1832633918
PR Review Comment: https://git.openjdk.org/jdk/pull/17331#discussion_r1459021353