HCR Redefinition of empty constructors gets ignored

[IBMJimmyk]

If an object has an empty constructor, it gets skipped over and its super constructor is called instead. This is a problem if HCR kicks in and replaces the empty constructor with a non-empty constructor. I talked to @jdmpapin and he helped me track down this problem.

The test case looks like this:

import java.io.*;
import java.lang.instrument.*;

public class RedefEmptyInit {

    public static void main (String[] args) {
        RedefEmptyInit t = new RedefEmptyInit();

        System.out.println("Running warmup.");
        for (int i = 0; i < 100; i++) {
            t.runTest();
        }

        int result = t.runTest();
        System.out.printf("Before redefine Result: %d, Expected: 0\n", result);

        try {
            File classFile = new File("InitObject-Redef.class");
            byte[] byteArray = new byte[(int)classFile.length()];
            FileInputStream inputStream = new FileInputStream(classFile);
            inputStream.read(byteArray);
            RedefAgent.instru.redefineClasses(new ClassDefinition(InitObject.class, byteArray));
        } catch (Exception e) {
            System.out.println("***FAILED*** - Unable to set up testing.");
            return;
        }

        result = t.runTest();
        System.out.printf("After redefine Result: %d, Expected: 1\n", result);

        if (1 != result) {
            System.out.println("***FAILED*** - Unexpected result.");
        } else {
            System.out.println("PASSED");
        }
    }

    RedefEmptyInit() {
    }

    protected int runTest() {
        InitObject testObj = new InitObject();

        return testObj.intField;
    }
}

runTest creates a new object of type InitObject, reads and returns the value of intField.

public class InitObject {

    public int intField;

    InitObject() {
        //intField = 0;
    }

}

With the line commented out, the constructor is empty and the error will occur. If it is uncommented, the error with not happen. Either way, the value of intField is 0.

The redefined version looks like this:

public class InitObject {

    public int intField;

    InitObject() {
        intField = 1;
    }

}

The value of intField is now 1 after the redefinition.

What the test does is create an object of type InitObject and check that the value of intField is 0. It then redefines the InitObject class so that intField is now 1. It then creates a new object to confirm that.

A failed result looks like this:

java -javaagent:RedefAgent.jar -Xint RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 0, Expected: 1
***FAILED*** - Unexpected result.

java -javaagent:RedefAgent.jar -Xjit:"optlevel=warm,count=10,disableasynccompilation,limit={*runTest*}" -Xshareclasses:none -Xnoaot RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 0, Expected: 1
***FAILED*** - Unexpected result.

Both Xint and JIT enabled fail to pick up the redefined class so after the redefinition it still reads intField as 0.

In the JIT'd code, the compiled method looks like this:

n1n       BBStart <block_2>                                                                   [    0x7c8d9cb546a0] bci=[-1,0,42] rc=0 vc=0 vn=- li=- udi=- nc=0
n5n       treetop                                                                             [    0x7c8d9cb547e0] bci=[-1,0,42] rc=0 vc=0 vn=- li=- udi=- nc=1
n4n         new  jitNewObject[#91  helper Method] [flags 0x400 0x0 ]                          [    0x7c8d9cb54790] bci=[-1,0,42] rc=3 vc=0 vn=- li=- udi=- nc=1
n3n           loadaddr  InitObject[#404  Static] [flags 0x18307 0x0 ]                         [    0x7c8d9cb54740] bci=[-1,0,42] rc=1 vc=0 vn=- li=- udi=- nc=0
n6n       allocationFence on n4n (X==0 X>=0 X<=0 )                                            [    0x7c8d9cb54830] bci=[-1,0,42] rc=0 vc=0 vn=- li=- udi=- nc=0 flg=0x302
n8n       NULLCHK on n4n [#32]                                                                [    0x7c8d9cb548d0] bci=[-1,4,42] rc=0 vc=0 vn=- li=- udi=- nc=1
n7n         call  java/lang/Object.<init>()V[#406  special Method] [flags 0x500 0x0 ]         [    0x7c8d9cb54880] bci=[-1,4,42] rc=1 vc=0 vn=- li=- udi=- nc=1
n4n           ==>new
n9n       astore  <auto slot 1>[#407  Auto] [flags 0x7 0x0 ]                                  [    0x7c8d9cb54920] bci=[-1,7,42] rc=0 vc=0 vn=- li=- udi=- nc=1
n4n         ==>new
n12n      NULLCHK on n10n [#32]                                                               [    0x7c8d9cb54a10] bci=[-1,9,44] rc=0 vc=0 vn=- li=- udi=- nc=1
n11n        iloadi  InitObject.intField I[#408  Shadow +8] [flags 0x603 0x0 ]                 [    0x7c8d9cb549c0] bci=[-1,9,44] rc=2 vc=0 vn=- li=- udi=- nc=1
n10n          aload  <auto slot 1>[#407  Auto] [flags 0x7 0x0 ]                               [    0x7c8d9cb54970] bci=[-1,8,44] rc=1 vc=0 vn=- li=- udi=- nc=0
n13n      ireturn                                                                             [    0x7c8d9cb54a60] bci=[-1,12,44] rc=0 vc=0 vn=- li=- udi=- nc=1
n11n        ==>iloadi
n2n       BBEnd </block_2>                                                                    [    0x7c8d9cb546f0] bci=[-1,12,44] rc=0 vc=0 vn=- li=- udi=- nc=0

Note how the constructor is a call to java/lang/Object.<init>()V with no guard to check for redefinition.

If the line is uncommented in the pre-redefined InitObject class, the test will pick up the redefinition and pass:

java -javaagent:RedefAgent.jar -Xint RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 1, Expected: 1
PASSED

java -javaagent:RedefAgent.jar -Xjit:"optlevel=warm,count=10,disableasynccompilation,limit={*runTest*}" -Xshareclasses:none -Xnoaot RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 1, Expected: 1
PASSED

The cause of the problem appears to be forwarder methods. https://github.com/eclipse-openj9/openj9/blob/881038dfc97ab68cf93dcce94812c48a2f47be3f/runtime/vm/resolvesupport.cpp#L1307 If J9_LOOK_ALLOW_FWD is removed there, the test will also pass.

Enabling extended HCR will cause the test to pass under Xint but not with the JIT:

java -javaagent:RedefAgent.jar -XX:+EnableExtendedHCR -Xint RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 1, Expected: 1
PASSED

java -javaagent:RedefAgent.jar -XX:+EnableExtendedHCR -Xjit:"optlevel=warm,count=10,disableasynccompilation,limit={*runTest*}" -Xshareclasses:none -Xnoaot RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 0, Expected: 1
***FAILED*** - Unexpected result.

Running instructions: Download: RedefEmptyInit.zip It contains 5 files:

RedefAgent.jar - Agent to help with redefinition
RedefEmptyInit.java - Test case
InitObject.java - Class to be redefined
InitObject-Redef.class - Class file to use with redefinition
InitObject-Redef.java - Source code of class file to use for redefinition

Commands:

javac -cp RedefAgent.jar InitObject.java RedefEmptyInit.java

java -javaagent:RedefAgent.jar -Xint RedefEmptyInit
java -javaagent:RedefAgent.jar -Xjit:"optlevel=warm,count=10,disableasynccompilation,limit={*runTest*}" -Xshareclasses:none -Xnoaot RedefEmptyInit

This was run on ppcle JDK11 but the problem should be common.

[jdmpapin]

Enabling extended HCR will cause the test to pass under Xint

I think this is because with extended HCR the redefinition will unresolve constant pool entries related to the redefined class

[gacholio]

I suspect this issue is not limited to constructors - we use the forwarding option for all invokespecial targets. While it would be foolish to have forwarder methods in general (the forwarding constructors are added by the compiler, I believe), it's certainly not illegal.

Two obvious options occur:

1) Remove the optimization - if the JIT is doing its job, this would only affect the interpreter.

2) Re-resolve invokespecial constant pool entries after normal HCR.

[jdmpapin]

(1) would fix the problem for both the interpreter and the JIT

(2) would fix the interpreter only, but the JIT would be straightforward. We'd just have to avoid J9_LOOK_ALLOW_FWD when jitCompileTimeResolve is true. (If we want we could still allow forwarding if HCR is disabled or we're in debug mode, since redefinition in debug mode will discard the entire code cache)

I don't really have a preference. Either way, the JIT can't correctly go on making use of this forwarding, at least under the defaults. So the difference is mainly re-resolution implementation effort vs. interpreter performance impact

[gacholio]

I don't think your suggestion for (2) would work - there's no guarantee who resolves a CP entry (interp or JIT) so changing the resolve behaviour likely won't work reliably.

I think we should remove the optimization and do some perf testing. The JIT will presumably naturally optimize the chain of forwarder constructors, so this should only affect the interpreter.

[gacholio]

I see the JIT currently uses the forwarder bit in the ROM method. If that can be removed in favour of general inlining of trivial methods, we can get rid of a bunch of code and reclaim the bit.

[jdmpapin]

I think we should remove the optimization and do some perf testing. The JIT will presumably naturally optimize the chain of forwarder constructors, so this should only affect the interpreter.

Good plan

I see the JIT currently uses the forwarder bit in the ROM method. If that can be removed in favour of general inlining of trivial methods, we can get rid of a bunch of code and reclaim the bit.

The JIT won't be able to use the forwarder bit at all, at least in HCR mode. Since HCR is on by default, I think from a JIT point of view it would be fine to remove the entire forwarding mechanism

[jdmpapin]

I don't think your suggestion for (2) would work - there's no guarantee who resolves a CP entry (interp or JIT) so changing the resolve behaviour likely won't work reliably.

Hopefully this is irrelevant, but I do think that (2) might work, and if not it could probably be made to work without too much trouble. The JIT calls jitResolveSpecialMethodRef() regardless of whether or not the CP entry is resolved. Then I don't think I see anything in there that short-circuits based on the state of the CP entry

[gacholio]

If no one else picks this up, I'll remove the entire mechanism in the next week or so and get some builds for perf testing.

[gacholio]

@jdmpapin Please review the JIT changes above to make sure I haven't removed something important.

[jdmpapin]

The commit deletes the beginning of a conditional:

if((subMethod != superMethod) && methodModifiersAreSafe  && ...)
   {
   if (traceIt)
      ...

I think we need to delete that entire case, all the way to

else if (superMethod != subMethod)

and then make that else if into just if

The first conditional is trying to analyze the bytecode to see when a method just forwards to the corresponding method in the base class. It's trying to avoid setting the bit that says that the base method is overridden and avoid invalidating nonoverridden runtime assumptions for it. But if we do that, then a later redefinition of the subclass could change the bytecode for this method and the JIT-compiled code would continue running the base implementation. The HCR assumptions would only guard against redefinition of the base class

[gacholio]

This is the current version of the change: https://github.com/gacholio/openj9/tree/forwarder

[jdmpapin]

The JIT changes LGTM, though the comment on if (superMethod != subMethod) could stand to be updated to match, since now there won't have been any attempt to inspect the bytecode

[gacholio]

Done.

[gacholio]

@IBMJimmyk Before sending this off for performance testing, can you please verify that this fixes the reported problem?

[IBMJimmyk]

With the latest changes, my test now passes. Both -Xint and -Xjit.

java -javaagent:RedefAgent.jar -Xint RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 1, Expected: 1
PASSED

java -javaagent:RedefAgent.jar -Xjit:"optlevel=warm,count=10,disableasynccompilation,limit={*runTest*}" -Xshareclasses:none -Xnoaot RedefEmptyInit
Running warmup.
Before redefine Result: 0, Expected: 0
After redefine Result: 1, Expected: 1
PASSED

[gacholio]

Thanks, I'll get this out for perf testing ASAP.

[gacholio]

Perf issue: https://github.ibm.com/runtimes/javanext/issues/466