TestFrameworkDriver simplification

[iignatev]

Hi Christian,

I've taken the 1st stub at simplifying TestFrameworkDriver. as you can see, now we can just have @run driver $Test in the tests w/ $Test::main being as simple as TestFrameworkDriver.main(args). to make this work, I, unfortunately, have to create a "packagefull" version of ClassFileInstaller (so it can be called from packages other than default). I haven't thoroughly tested that change, in other words, there can be tests that would start to fail b/c of that, and I am not 100% sure about the location of ClassFileInstaller, I actually was thinking about putting it (as well as RedefineClassHelper) into jdk.test.lib.helpers. in any case, we can discuss that later.

Cheers,
-- Igor

[chhagedorn]

Hi Igor

Thanks a lot for trying this out! This simplified way to write a jtreg test is great! I took your change and tried to update the framework such that the user is not even aware of the driver and can just directly set the things up in the main method of a test as before (as for example in TestSanity).

To achieve that I split the TestFramework up into a TestFrameworkRunner and TestFrameworkExecution (names can still be changed). I then renamed the TestFrameworkDriver into TestFramework to get it as interface and driver. Now it works as follows:

• TestSimpleExample calls TestFramework.run()
• TestFramework acts as driver and starts a new "framework runner VM" with all the flags to enable the WB. It also acts as an interface to the framework and provides all the interface methods that were in the old TestFramework class. All the methods that require WB support are delegated to TestFrameworkExecution but these should only be used in the actual test methods ( @Test, @Run etc.) and not in the main() method of a test (there is an exception message that warns about that: link). So, this should not be a problem. The TestFramework now sets up helper classes, scenarios etc. and passes everything as arguments to the new "framework runner VM" with WB support which calls TestFrameworkRunner.main().
• TestFrameworkRunner now has WB support and can set up all flags that are required to do the IR matching. It starts the "test VM" to execute the tests which calls TestFrameworkExecution.main()
• TestFrameworkExecution then eventually runs all the tests and prints everything needed for TestFrameworkRunner to then do the IR matching in the "framework runner VM" once the "test VM exits".

I also renamed the package according to your suggestion to jdk.test.lib.hotspot.ir_framework. There is quite some VM nesting now to eventually run the tests and do the matching. I hope that is not a problem. I haven't updated the internal framework tests, yet, to also use jtreg (jdk.test.lib.hotspot.ir_framework.tests). But TestSimpleExample works now.

Here is the diff to the previous version: Update vs. previous version

What do you think?

Cheers,
Christian

[iignatev]

LGTM. re: ClassFileInstaller, I'll go ahead and open an RFE against the mainline to have it in a package, so it will be easier for you to integrate.

[iignatev]

btw, reading the framework code(just to be clear I read only some of it and not very thoroughly, so I might misintererpted your intention), I noticed this: cmds.addAll(Arrays.asList(InputArguments.getVmInputArgs()));, I supposed you wanted to use this to propagate "external" VM flags (i.e. jtreg's vmoptions) to JDK under test, however b/c you used @run driver it won't do that, InputArguments.getVmInputArgs will be (almost) empty, can't you use some of ProcessTools methods there to start JVM?

[chhagedorn]

LGTM. re: ClassFileInstaller, I'll go ahead and open an RFE against the mainline to have it in a package, so it will be easier for you to integrate.

Thank you Igor for your review! That sounds good, thanks for taking care of the ClassFileInstaller.

btw, reading the framework code(just to be clear I read only some of it and not very thoroughly, so I might misintererpted your intention), I noticed this: cmds.addAll(Arrays.asList(InputArguments.getVmInputArgs()));, I supposed you wanted to use this to propagate "external" VM flags (i.e. jtreg's vmoptions) to JDK under test, however b/c you used @run driver it won't do that, InputArguments.getVmInputArgs will be (almost) empty, can't you use some of ProcessTools methods there to start JVM?

I see, that is a problem. Yes, I just wanted to pass all flags that would normally be passed to a compiler jtreg test with main/othervm, either specified at the test or passed over jtreg options. Thinking about it, could I just change it to main/othervm instead like @run main/othervm -XX:-LoopUnswitching compiler.valhalla.testframework.TestSimpleExample? That seems to work.

[iignatev]

changing it even to simple @run main will work b/c all flags are passed to the VM which runs TestFramework, but, as far as I understand the code, we don't want to run that VM w/ all external flags as it doesn't actually do any testing and just "glues" the test and verification together. running that VM w/ all stress flags would just result in a waste of time to no gain. that's to say, I think we have to use run driver to run the framework code, you should be able to achieve that you need w/ existing jdk.test.lib.* classes:

ProcessTools::executeTestJvm(String[] cmds) prepends all external flags to cmds, i.e. it executes ${test.jdk}/bin/java ${test.vm.opts} ${test.java.opts} ${cmds}, if you need more control you can use ProcessTools::createJavaProcessBuilder together w/ Utils::prependTestJavaOpts/apppendTestJavaOpts, or Utils::getTestJavaOpts/getFilteredTestJavaOpts if you need do smth weird.

[chhagedorn]

You're right, that's a useless exercise to run the VM which calls TestFramework.run() from TestSimpleExample with all the additional stress flags. It's also not necessary to run the "TestFramework runner" VM with the user set scenario flags, yet (which it currently does). Moreover, the "TestFramework runner" VM, which sets up all flags for the test VM, also does not need the external flags to be stressed but only the test VM itself that is called here.

I updated the TestFramework to only add the vm and java options again as property flags. I haven't found something in Utils or ProcessTools, so I added a new function for it. The user defined scenario flags are passed as -DScenarioFlags and added in the "TestFramework runner" VM to the test VM. I also cleaned up some unused code left over from the split.

Changes: openjdk@b011f22

[iignatev]

why do we need to run TestFramework in a separate VM? is it b/c it needs to be run w/ WhiteBox?

also, does JVM under test have to be created by "TestFramework runner" VM, ie can't it be initiated by the 1st "driver" VM?

[chhagedorn]

Yes, the "TestFramework runner" VM in TestFrameworkRunner needs the WB to decide which flags to use for the test VM (example). Therefore, the first "driver" VM in TestFramework first needs to start the "TestFramework runner" VM with WB enabled which can then set up all flags for the test VM.

[iignatev]

ok, I think it would be easier to understand if "TestFramework runner" VM did its job of query JVM and constructing (partial?) command line, passed it back to "driver" VM, and "driver" VM took care of spawning "test" VM.

and it actually seems that "TestFramework runner" VM has to be run w/ all external flags, otherwise, the flags' value won't be correct.

[chhagedorn]

(Sorry, I somehow forgot to hit "Comment" yesterday)

I agree, that's a much better idea instead of having the test VM nested in the runner VM nested in the driver VM. And you're also right, we need the external flags to accurately prepare the test VM flags.

I updated that such that we are now calling a "flag" VM with WB access from the driver VM in TestFramework.runFlagVM().
The flag VM just emits all the flags that are required for the test VM to the standard output in TestFrameworkPrepareFlags.emitTestVMFlags() (renamed from TestFrameworkRunner). The driver VM then parses them in TestFramework.getTestVMFlags() and adds them to the test VM which it then starts in TestFramework.runTestVM()

So, we have this workflow now:
driver VM (TestFramework) -> flag VM (TestFrameworkPrepareFlags) -> driver VM -> test VM (TestFrameworkExecution) -> driver VM doing IR verification

Here is the updated version (relevant changes in TestFramework and TestFrameworkPrepareFlags): openjdk@152effc

Cheers,
Christian

[iignatev]

Hi Christian,

sounds good to me, one thing though: instead of using stdout to pass flags from "flag" VM to "driver" VM, I'd recommend you to use a dedicated file, socket, etc. that will make this communication more robust and independent of the will of JVM to produce warning/error or any other messages to stdout.

[chhagedorn]

Thanks Igor! That sounds better instead of relying on the stdout. I tried a socket implementation for the flag VM and also reused it to pass the encoding from IREncodingPrinter from the test VM back to the driver VM (using TestFrameworkSocket.write() for both cases). Do I need to select a specific port number or host name to get it working with Mach5? I've just used a random port and localhost which seems to work locally for the tests that I have.

The relevant changes are around TestFrameworkSocket inside TestFramework.java:
openjdk@db7c409

Cheers,
Christian

[iignatev]

it would seem you forget to git add TestFrameworkSocket.java, at least I can't find it in your TestingFramework branch.

[chhagedorn]

Hi Igor, it's inside the file TestFramework.java: TestFrameworkSocket. I've just seen now that it was unlucky that Git did not expand the file TestFramework.java by default in the commit openjdk@db7c409 and hid the class TestFrameworkSocket well.

[iignatev]

strange, I used search in the repo functionality and it didn't find anything... anyhow, you shouldn't use a specific port as there is no guarantee that it will be available, what you should do instead is to use an ephemeral port when you create a socket and pass it as an argument/property to "flag" VM:

    TestFrameworkSocket() {
        try {
            serverSocket = new ServerSocket(0);
        } catch (IOException e) {
            TestFramework.fail("Server socket error", e);
        }
    }

instead of using "localhost" as hostname, you can pass null to Socket::Socket(String, int), which would be equivalent of connection to loopback i-face:

// that's if you decide to go w/ property
static final String SERVER_PORT_PROPERTY = "jdk.test.lib.hotspot.ir_framework.port";
static final int SERVER_PORT  = Integer.getInteger(SERVER_PORT_PROPERTY);
...
public static void write(String msg, String type) {
        try (Socket socket = new Socket(null, SERVER_PORT);
             PrintWriter out = new PrintWriter(socket.getOutputStream(), true)) {
            out.print(msg);
        } catch (Exception e) {
            TestFramework.fail("Failed to write to socket", e);
        }
        if (TestFramework.VERBOSE) {
            System.out.println("Written " + type + " to socket:");
            System.out.println(msg);
        }
    }

[chhagedorn]

Yeah, I thought that it's probably not a good idea to use a hardcoded port. Thanks for the pointers. I adapted that approach and pass the port as property to the flag and test VM and use null for the host.

Here are the changes, the relevant parts are again around TestFrameworkSocket: openjdk@fc13fb8

[iignatev]

looks good. I'll try to find time next week to go thru the code as a whole. does TestingFramework have the latest code?

[iignatev]

LGTM. re: ClassFileInstaller, I'll go ahead and open an RFE against the mainline to have it in a package, so it will be easier for you to integrate.

Thank you Igor for your review! That sounds good, thanks for taking care of the ClassFileInstaller.

8263412 (and its follow-up fixes) moved ClassFileInstaller to jdk.test.lib.helpers in the mainline.

[chhagedorn]

looks good. I'll try to find time next week to go thru the code as a whole. does TestingFramework have the latest code?

Thanks a lot! I'm currently changing some things in TestFrameworkExecution.java and in some of the annotations to write tests. But I have not touched TestFramework.java anymore (if you just want to take a look at this file for the VM setup etc.). So, the latest commit contains the most recent version of TestFramework.java. Otherwise, I'll let you know when I pushed the current ongoing changes in the other files (probably tomorrow).

LGTM. re: ClassFileInstaller, I'll go ahead and open an RFE against the mainline to have it in a package, so it will be easier for you to integrate.

Thank you Igor for your review! That sounds good, thanks for taking care of the ClassFileInstaller.

8263412 (and its follow-up fixes) moved ClassFileInstaller to jdk.test.lib.helpers in the mainline.

Great!

[chhagedorn]

Hi Igor, I pushed the updates yesterday and just before. I'm in the process of adding now more documentation to the code which I will push incrementally. You can already take a look at the code or wait for some more documentation. There is no hurry I think. Thanks again for all your help, I highly appreciate it!