Openj9: Problem with OSR from inlined synchronized method

[cathyzhyi]

We are having a problem with OSR from inlined synchronized method. The solutions discussed offline have pros and cons so I am documenting it here for discussion and future references.

When a synchronized method exits through unhandled exception, it's required by the specification to release the monitor before exits. The VM unwind logic will handle this for synchronized methods that are not inlined. For inlined ones, JIT inserts a synthetic handler to catch the exception and do monitorExit and then rethrow the exception. This gives JIT the flexibility to do monitor optimizations for monitors for inlined synchronized methods. However, the problem comes when OSR happens inside the handler, because it doesn't exist in bytecodes. More specifically, OSR can happen at:

• breakOnCatch at the start of the handler
• at monexit when throwing IllegalMonitorStateException
• at the rethrow.

We need a way for JIT and VM to agree on the OSR state. There are 2 main directions to solve the problem:

• Keep the current way of inserting synthetic JIT handlers but we need a special way to make VM and JIT agree on the OSR state. There are 2 ways to do this as well:
  
  
  
  • We can assign a special bytecode index to the GC points in the handler so that on OSR VM will know the monitor exit to the handler PC to indicate to VM this handler is for unwinding inlined synchronized method and do the right things. To avoid surprising the rest of the compiler this should be done as late as possible at metadata encoding stage. However, it's still fragile as future code will need to always keep the special bytecode index in mind to avoid introducing bugs.
  
  
  • We can encode the special information properly in the metadata. However, changing metadata's shape is also something we need to be very careful about. I would need suggestions on what is the best way to encode the information.
  
  
• Let VM handle unwinding of inlined synchronized method. This prevents JIT from "desynchronize" inlined method so it is currently not the direction we are focusing on.

@vijaysun-omr @fjeremic @gita-omr do you have any suggestions on this?
FYI @gacholio @DanHeidinga @andrewcraik

[gacholio]

Would this be solved by changing all of the explicit -1 checks (or negative checks) into a helper (e.g. validateBytecodeIndex would know all of the explicit special cases)?

As far as how the data is encoded, I don't really mind. I'm currently using the stackmap/inlinemap to get the bcindex, so the data must be available from there, or some new API based on the PC.

[gacholio]

Also, for the record, my second proposal of "let the VM do it" will require at least as much metadata work, so I'm no longer thinking that's a viable short-term fix.

[vijaysun-omr]

@cathyzhyi what is the bytecode index used on the monexit added in the catch block ? If the bytecode index was the one corresponding to one of the return bytecodes in that method, would something go wrong ? Maybe this requires us to consider return bytecodes for synchronized methods as potential OSR points in the JIT's OSR analyses but this can be managed by changing the logic in those cases fairly easily I imagine.

[gacholio]

@vijaysun-omr - we simply cannot have an exception handler that doesn't exist in bytecodes (which is why we're trying to hide these ones). The bytecode index really doesn't matter.

[gacholio]

Assuming this isn't days of work, can I suggest that we prototype the proposed solution to see if it fixes the issue? The VM side is done (assuming we go with -2 as the index):

https://github.com/gacholio/openj9/commit/a9638940db5665159ef24564afb9903465f4a8a6

[vijaysun-omr]

I was responding to the proposal from Yi that "We can assign a special bytecode index to the GC points in the handle...", i.e. why do we need a special bytecode index when we might be able to reuse the bytecode index of the return bytecode (that is not an OSR point otherwise) to "signal" to the VM that special handling is needed.

[gacholio]

We need a unique value that cannot be a valid bytecode index to tag the artificial handlers. If we used the return bytecode index, the VM would continue to build the stack incorrectly.

[gacholio]

We get the OSR block from a call to preOSR. I suppose that could return something magic to indicate OSR isn't possible (at the moment, the VM assumes that OSR is always possible when requested).

Having said that, you'd have to scan for a return bytecode (it's not necessarily the last bytecode in the method). Feels a bit hacky to me.

[andrewcraik]

I agree with @gacholio if the block is a fiction and needs to be recognized as such I don't think it can/should have a valid BCI.

[fjeremic]

Would this be solved by changing all of the explicit -1 checks (or negative checks) into a helper (e.g. validateBytecodeIndex would know all of the explicit special cases)?

If we go down the road of using a special bytecode index value to identify this case I'd very much support this idea. I'm concerned about the use of special values as the rest of the codebase knows nothing about this.

[gacholio]

Any update on the JIT side of this?

[cathyzhyi]

Any update on the JIT side of this?

@gacholio We've had some offline discussion and there is another idea that we can probably encode the information in OSRMetadata. However, currently, VM side doesn't deal with OSRMetadata at all right?

I am still working on encoding the special bytecode index value into instruction metadata so that we can get a version working at least.

[gacholio]

No I never deal with the metadata. If an API is provided in MethodMetadata, I'm sure I could easily use it.

[gacholio]

Thinking about the requirements inside the artificial handler if it's going to throw IllegalMonitorStateException:

Java stack      Native stack

exception constructor
callin frame
            callin code
resolve frame
            jitMonitorExit code
fake handler

The call-in can not be bypassed by the debugger, so no matter what, we're going to throw something on return from the callin, meaning we never resume in the fake handler in the exception case.

I am assuming that each fake handler is for a single monitor (i.e. the JIT has converted inline sync methods into monent; try { inlined code; } finally { monexit; }).

It is currently possible (though never used) for monexit to report an event, so the stack should be sensible when we do so.

So, the complete fix is:

1) Do not report catch for fake handlers (via bcIndex or metadata)
2) After successful monexit in the fake handler, re-throw with jitThrowUnreportedException

These two keep the throw/catch event reporting consistent and prevent OSR at the start of the fake handler. OSR will not occur within the fake handler because the monexit helper either completes with no possibility of OSR, or does not return to the fake handler).

3) Assign bcIndex to the monexit helper call

Put a proper inline map on the monexit helper call and assign a valid bcIndex to the top inline frame (I think 0 is really the only choice here) - the further inlines should report their invoking bcIndex. This keeps the stack sane (for methods if not PCs) If we do report an event on the monexit. Given that we never hook this event (and I may delete it), 0 seems a fine choice for the top bcIndex.

@cathyzhyi @andrewcraik What do you think?

[gacholio]

The core assumption here is that it's one fake handler per monitor - if we have to handle the case of one monexit succeeding followed by another one failing (in the same handler), this becomes much more complicated.

[cathyzhyi]

1. Assign bcIndex to the monexit helper call
   Put a proper inline map on the monexit helper call and assign a valid bcIndex to the top inline frame (I think 0 is really the only choice here)

Just to make sure I understand this correctly. When monexit throws IllegalMonitorStateException from synthetic handler and OSR, VM resumes from the next handler to handle this new exception. The bytecode index for monexit is only for the purpose of keeping the stack shape sane but not where to resume execution so it's ok to assign bytecode index 0 here.

The core assumption here is that it's one fake handler per monitor - if we have to handle the case of one monexit succeeding followed by another one failing (in the same handler), this becomes much more complicated.

Yes, there is only one synthetic handler per monitor.

[gacholio]

Correct.

[cathyzhyi]

We find another possible way to encode the info. In the exception table, the_catchType for the handler is the cpIndex of the exception class with 0 meaning catching all exceptions. Value 0xffff/0xffffffff (for 2bytes exception entry and 4bytes exception entry mode) can be reserved to indicate synthetic handler. The synthetic handlers can catch all types so VM can change it to 0 when they see 0xffff/0xffffffff. https://github.com/eclipse/openj9/blob/2cc3b1cf81180cc7a45817e8631de00681f99372/runtime/compiler/runtime/MetaData.cpp#L171-L196
In this way, the shape of the metadata won't be changed.
A cleaner way would be allocating a dedicated field of uint8 to indicate the synthetic handler. However, we would be using 1 bit only at this moment and I am not sure how it interferes with the rest of method metadata logic.
@vijaysun-omr @andrewcraik do you have any comments?

[vijaysun-omr]

I am okay with this but the VM side view has to come from @gacholio here obviously and we should amend our design if he does not like this approach.

From a JIT perspective, I prefer this kind of "signal" value (and where we write it) to be as localized (and delayed) in the compiler as possible, so that we do not have to teach different parts of the compiler about "special" values (which is the point I was trying to make earlier). This meets those criteria of being quite late and quite localized in my personal opinion.

[gacholio]

I also like this approach, as I can easily detect the magic value in MethodMetadata.c and change it to 0. At the same time. I can tag a field in the walkState to indicate to the exception catch code that this is a synthetic handler.

Something will need to be done to ensure that the reserved value is never used as a real index (i.e. fail compilation in that case).

[gacholio]

Looks like the VM code isn't quite right - the test works with the new changes, but it's not decompiling at the catch point as expected. I'm looking into this.

[gacholio]

I've got the expected case working, now attempting to make the IllegalMonitorStateException case work.

[gacholio]

Both of the throw cases from inside the handlers are now working (with the appropriate changes from @cathyzhyi ).