ibmruntimes / openj9-openjdk-jdk

Extensions for OpenJDK for Eclipse OpenJ9
GNU General Public License v2.0
17 stars 75 forks source link

Initialize VarHandle.methodHandleTable and its entries with atomic CAS #747

Closed jdmpapin closed 8 months ago

jdmpapin commented 8 months ago

This prevents double initialization when multiple threads are racing through getMethodHandle(). Once a non-null reference has been written to one of these locations, any subsequent write to the same location is a problem for the OpenJ9 JIT given its current handling of known objects.

Issue: eclipse-openj9/openj9#18882

keithc-ca commented 8 months ago

a problem for the OpenJ9 JIT given its current handling of known objects

I'm not sure the problem is confined to OpenJ9: the assertion is generated by InvokerBytecodeGenerator.

jdmpapin commented 8 months ago

Yes, but a subsequent write only causes the assertion to fail because of the way we deal with known objects. Otherwise, we'd have gotten the LF from the MH at runtime and it would match

keithc-ca commented 8 months ago

Yes, but a subsequent write only causes the assertion to fail because of the way we deal with known objects. Otherwise, we'd have gotten the LF from the MH at runtime and it would match

I'm not sure whether to infer from that response that future changes to OpenJ9 are anticipated which will eliminate the need for this patch.

jdmpapin commented 8 months ago

Yes, we anticipate future OpenJ9 changes will allow this to be reverted, in particular the implementation of https://github.com/eclipse-openj9/openj9/issues/16616. See https://github.com/eclipse-openj9/openj9/issues/18882#issuecomment-1958173709 and https://github.com/eclipse-openj9/openj9/issues/18882#issuecomment-1960277863. It will be some time though. I have some not-yet-contributed partial progress toward it, but none of that is on the code generation / GC interactions, which I expect to be the bulk of the work

jdmpapin commented 8 months ago

Updated to address review comments

keithc-ca commented 8 months ago

Jenkins test sanity.openjdk zlinux jdknext

keithc-ca commented 8 months ago

Jenkins copyright check

keithc-ca commented 8 months ago

The failures seem to only be with respect to java.lang.Class.toGenericString() because sealed is missing; see https://github.com/eclipse-openj9/openj9/issues/19033.