RFR 8224137: Analyze and port invocation tests to jtreg and co-locate to jdk repo

[David Holmes]

On 25/06/2019 8:53 am, Harold Seigel wrote:
> Hi David,
> 
> Please see comments inline.
> 
> On 6/24/2019 4:15 PM, David Holmes wrote:
>> Hi Harold,
>>
>> On 24/06/2019 1:04 pm, Harold Seigel wrote:
>>> Hi David,
>>>
>>> Thanks for pointing this out.  My intent is to push the current tests 
>>> and then create an enhancement to improve them.  I'd like to add C1 
>>> and Graal testing as part of the improvements.
>>
>> Okay. Wasn't sure why you went for the -Xint / -Xcomp additions at 
>> this stage? :)
> I plan to also test c1 and graal but would like to start with these.
>>
>>> Also, note that passing TRUE as the first argument to 
>>> ProcessBuild.createJavaProcessBuilder() adds test.vm.opts and 
>>> test.java.opts to the java arguments.
>>
>> Okay so your explicit Xint and Xcomp can conflict with what gets 
>> passed through by jtreg. I'm not sure which order flags will be 
>> presented, so even if "last flag wins" it's not clear exactly what 
>> will get tested. In particular using Xcomp when jtreg passes in Graal 
>> flags, can cause issues.
> Is there an open bug for the Xcomp / Graal flags issue?

It isn't a bug per-se, simply that if you have certain flags on an @run 
or explicit exec of the VM, to try and test C1/C2, if you then apply the 
Graal flags on top, it causes Graal to execute in a slow mode that leads 
to timeouts. So a number of tests have been modified. May take me a 
while to find the details ...

>>
>> So maybe for this initial push explicit mode selection should be avoided?
> 
> I think it would be better to pass FALSE as the first argument to 
> ProcessBuild.createJavaProcessBuilder(). Then I will know what is 
> getting tested.

That's a good/safe starting point.

Thanks,
David

> Thanks, Harold
> 
>>
>> Thanks,
>> David
>> -----
>>
>>> Harold
>>>
>>> On 6/21/2019 9:18 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> Not a review ...
>>>>
>>>> On 21/06/2019 10:55 am, Harold Seigel wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Can you take one more look?
>>>>>
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137.3/webrev/
>>>>>
>>>>> I changed three files, invokespecialTests.java, 
>>>>> invokeinterfaceTests.java, and invokevirtualTests.java to 
>>>>> explicitly specify -Xint and -Xcomp to the sub-tests.
>>>>
>>>> If you want to check all the compilation environments you need to be 
>>>> more elaborate than just using -Xcomp. You may need to test C1 only 
>>>> and C2 only. And then you need to consider Graal as well.
>>>>
>>>> IIUC from a brief look at these tests they will never pass through 
>>>> the VM flags given to jtreg - is that correct?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Thanks again!
>>>>>
>>>>> Harold
>>>>>
>>>>> On 6/20/2019 6:37 PM, coleen.phillimore at oracle.com wrote:
>>>>>> Great, thanks. Looks good!
>>>>>> Coleen
>>>>>>
>>>>>> On 6/20/19 1:00 PM, Harold Seigel wrote:
>>>>>>> Hi Coleen,
>>>>>>>
>>>>>>> Thanks for reviewing this change!
>>>>>>>
>>>>>>> Please review this updated webrev:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137.2/webrev/index.html
>>>>>>>
>>>>>>> The -XDignore.symbol.file has been removed and TEST.groups has 
>>>>>>> been changed.  Otherwise, it is the same as the previous webrev.
>>>>>>>
>>>>>>> Mgmt said to leave the copyright years as 2009, 2019, so I did 
>>>>>>> not change those.
>>>>>>> Thanks! Harold
>>>>>>>
>>>>>>> On 6/19/2019 5:41 PM, coleen.phillimore at oracle.com wrote:
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137/webrev/test/hotspot/jtreg/runtime/InvocationTests/invokeinterfaceTests.java.html 
>>>>>>>>
>>>>>>>>
>>>>>>>>   35  * @compile -XDignore.symbol.file 
>>>>>>>> invokeinterface/Checker.java invokeinterface/ClassGenerator.java
>>>>>>>>
>>>>>>>>
>>>>>>>> Do these still need -XDignore.symbol.file ?
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137/webrev/test/hotspot/jtreg/TEST.groups.udiff.html 
>>>>>>>>
>>>>>>>>
>>>>>>>> Can you specify all the tests in the directory by directory? like:
>>>>>>>>
>>>>>>>> + -runtime/InvocationTests \
>>>>>>>>
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137/webrev/test/hotspot/jtreg/runtime/InvocationTests/shared/Caller.java.html 
>>>>>>>>
>>>>>>>>
>>>>>>>>    2  * Copyright (c) 2009, 2019, Oracle and/or its affiliates. 
>>>>>>>> All rights reserved.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Some of the copyrights say 2009, which is when the tests were 
>>>>>>>> written but I think we're supposed to have the original date 
>>>>>>>> when they're added to the repository.
>>>>>>>>
>>>>>>>> Well done getting these tests into jtreg and the repository!
>>>>>>>> Thanks,
>>>>>>>> Coleen
>>>>>>>>
>>>>>>>> On 6/17/19 2:57 PM, Harold Seigel wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Please review this JDK-14 change to move the invocation tests 
>>>>>>>>> written by Vladimir Ivanov into the JDK repo and make them 
>>>>>>>>> runnable using JTReg.
>>>>>>>>>
>>>>>>>>> This webrev adds three tests, invokeinterfaceTests.java, 
>>>>>>>>> invokespecialTests.java, and invokevirtualTests.java. Each 
>>>>>>>>> tests run its set of sub-tests twice, once using class file 
>>>>>>>>> version 51 and once using the current class file version.
>>>>>>>>>
>>>>>>>>> Open Webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~hseigel/bug_8224137/webrev/
>>>>>>>>>
>>>>>>>>> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8224137
>>>>>>>>>
>>>>>>>>> The tests were tested on Linux-x64, Solaris, Windows, and Mac 
>>>>>>>>> OS X.
>>>>>>>>>
>>>>>>>>> The original tests can be found attached to JDK-8163974 
>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8163974>. Besides the 
>>>>>>>>> changes needed for JTReg and adding copyrights, I made the 
>>>>>>>>> following additional changes.
>>>>>>>>>
>>>>>>>>> 1. The tests now use the JDK's asm support instead of providing 
>>>>>>>>> its own
>>>>>>>>>    asm libraries.
>>>>>>>>> 2. Only sub-test failures are written to the .jtr files. 
>>>>>>>>> Writing all
>>>>>>>>>    sub-test results caused JTReg to truncate the output.
>>>>>>>>> 3. Changed src/invokeinterface/Checker.java to skip private 
>>>>>>>>> methods
>>>>>>>>>    when looking for an overloading method.
>>>>>>>>>
>>>>>>>>> The tests contain "TODO" comments and other thing needing 
>>>>>>>>> clean-up.  These will be addressed in a future RFE.
>>>>>>>>>
>>>>>>>>> I put the tests into hs-tier3 because the 
>>>>>>>>> invokeInterfaceTests.java test can run for up to 10 minutes (on 
>>>>>>>>> Mac).  The other two tests take only 1-2 minutes.  Is there a 
>>>>>>>>> better tier for these tests?
>>>>>>>>>
>>>>>>>>> Thanks, Harold
>>>>>>>>>
>>>>>>>>
>>>>>>