RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be updated to divide test jdk and compile jdk

Tue Sep 17 07:46:08 PDT 2013

Hi Christian,

The changes look good.

Thanks, Harold

On 9/16/2013 10:41 PM, David Holmes wrote:
> Thumbs up from me!
>
> Thanks,
> David
>
> On 17/09/2013 7:47 AM, Christian Tornqvist wrote:
>> Updated webrev where I've removed
>>
>> runtime/NMT/MallocTestType.java
>> runtime/NMT/ThreadedMallocTestType.java
>> runtime/NMT/ThreadedVirtualAllocTestType.java
>> runtime/NMT/VirtualAllocTestType.java
>> runtime/NMT/BaselineWithParameter.java
>> runtime/NMT/JcmdScale.java
>> runtime/NMT/JcmdWithNMTDisabled.java
>> runtime/NMT/ShutdownTwice.java
>> runtime/NMT/SummaryAfterShutdown.java
>> runtime/NMT/SummarySanityCheck.java
>> runtime/RedefineObject/TestRedefineObject.java
>>
>> from the needs_jdk group in TEST.groups based on David's comment in 
>> the bug.
>>
>> Webrev can be found at:
>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.04/
>>
>> Thanks,
>> Christian
>>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Sunday, September 15, 2013 9:45 PM
>> To: Christian Tornqvist
>> Cc: 'hotspot-dev developers'
>> Subject: Re: RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be 
>> updated to divide test jdk and compile jdk
>>
>> On 16/09/2013 11:41 AM, David Holmes wrote:
>>> On 14/09/2013 4:52 AM, Christian Tornqvist wrote:
>>>> I've added getCompileJDKTool() and getTestJDKTool(), the common
>>>> function getJDKTool() will still look in test.jdk first and
>>>> compile.jdk as a fallback.
>>>>
>>>> Changed the TestVerifyDuringStartup.java that Igor changed as part of
>>>> his change to use getJDKTool() since this is what
>>>> ProcessTools.createJavaProcessBuilder() does.
>>>>
>>>> Verified my change locally by running test/runtime and test/gc jtreg
>>>> tests to verify that they work correctly when test.jdk is set to a 
>>>> JRE.
>>>>
>>>> New webrev can be found at:
>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.03/
>>>
>>> Looks good to me!
>>>
>>> Thanks,
>>> Chris
>>
>> That should be:
>>
>> Thanks, Chris.
>>
>> David
>> -----
>>
>> David
>> -----
>>
>> :)
>>
>>>> Thanks,
>>>> Christian
>>>>
>>>> -----Original Message-----
>>>> From: hotspot-runtime-dev-bounces at openjdk.java.net
>>>> [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of
>>>> David Holmes
>>>> Sent: Friday, September 13, 2013 7:11 AM
>>>> To: Erik Helin
>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>> Subject: Re: RFR(S): 8014905 - [TESTBUG] Some hotspot tests should be
>>>> updated to divide test jdk and compile jdk
>>>>
>>>> Just to cc the list what I said in other email ...
>>>>
>>>> I think this is a good compromise. Having an API that allows the test
>>>> to select which JDK to use is important; but maintaining
>>>> compatability with existing test behaviour is also important.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 12/09/2013 11:22 PM, Erik Helin wrote:
>>>>> On 2013-09-12, David Holmes wrote:
>>>>>> On 12/09/2013 9:27 PM, Igor Ignatyev wrote:
>>>>>>> On 09/12/2013 02:13 PM, David Holmes wrote:
>>>>>>>> On 12/09/2013 8:07 PM, Igor Ignatyev wrote:
>>>>>>>>> On 09/12/2013 01:54 PM, David Holmes wrote:
>>>>>>>>>> On 12/09/2013 4:20 PM, Igor Ignatyev wrote:
>>>>>>>>>>> Christian,
>>>>>>>>>>>
>>>>>>>>>>> I have made some changes in JDKToolFinder w/ patch for
>>>>>>>>>>> JDK-8012447[*]:
>>>>>>>>>>>     - getJDKTool uses 'compile.jdk'
>>>>>>>>>>>     - getCurrentJDKTool uses 'test.jdk'
>>>>>>>>>>>
>>>>>>>>>>> So, I'm not sure that your change is necessary.
>>>>>>>>>>
>>>>>>>>>> I prefer Christian's approach as it is behaviour preserving.
>>>>>>>>>> With your change any test that actually wants/needs to use the
>>>>>>>>>> test JDK will have to be modified.
>>>>>>>>> I agree, but w/ Christian's approach we could get situation that
>>>>>>>>> we test binary from another JDK, e.g.
>>>>>>>>> 'test/gc/TestVerifyDuringStartup.java'
>>>>>>>>> which used 'java' from 'compile.jdk' instead of 'test.jdk'.
>>>>>>>>
>>>>>>>> I don't see how as the test JDK is checked first. The gap in
>>>>>>>> Christian's approach is the more rare case of where the test JDKs
>>>>>>>> tool must not be used for that part of the test.
>>>>>>> Oh sorry, I misread the code. Anyway, I don't think that this code
>>>>>>> is good, because if I want to get one of jdk tool, I expect that I
>>>>>>> will get it from '-compilejdk' if this option's specified, and
>>>>>>> from '-jdk', otherwise. But w/  Christian's patch I will get tool
>>>>>>> from '-jdk' if tool exists there, that may confuse.
>>>>>>
>>>>>> Anyone using this function has to know what it returns regardless
>>>>>> of whether that is from compile-jdk or test-jdk. With your change a
>>>>>> bunch of tests that were using the test-jdk tools will now be using
>>>>>> the compile-jdk tools, and while that might not cause a test
>>>>>> failure it might negate the purpose of the test. So unless you've
>>>>>> examined all the tests that use this API I can't agree that your
>>>>>> change was appropriate.
>>>>>
>>>>> I, Igor and Christian discussed this on IM and the solution that we
>>>>> came up with was that JDKToolFinder should have three methods:
>>>>> getJDKTool, getCompilerJDKTool and getTestJDKTool. getJDKTool
>>>>> should, as in Christian's change, first try to make use of
>>>>> getTestJDKTool and then, if that fails, use getCompilerJDKTool.
>>>>>
>>>>> getCompilerJDKTool and getTestJDKTool should be like the ones Igor
>>>>> has written, but the should use make use of a private function to
>>>>> avoid code duplication.
>>>>>
>>>>> This way, writing an simple test will still be easy, using
>>>>> getJDKTool should in most cases be what you want. However, if you
>>>>> are writing a more tricky test, getTestJDKTool and
>>>>> getCompilerJDKTool are still there for those cases.
>>>>>
>>>>> David, what do think about this suggestion?
>>>>>
>>>>>>>> So I think
>>>>>>>>> that any test that actually wants/needs to use the test JDK must
>>>>>>>>> do it explicitly.
>>>>>>>>>>
>>>>>>>>>>> [*]
>>>>>>>>>>> http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/ceda33
>>>>>>>>>>> ff
>>>>>>>>>>> 54a3
>>>>>>>>>>
>>>>>>>>>> Was this reviewed on hotspot-dev/hotspot-runtime-dev or only
>>>>>>>>>> hotspot-compiler-dev?
>>>>>>>>>
>>>>>>>>> It was reviewed only on hotspot-compiler-dev.
>>>>>>>>
>>>>>>>> These kinds of changes really have to be reviewed more broadly as
>>>>>>>> they affect everyone. We wouldn't even have known about your
>>>>>>>> change if Christian hadn't done this one. Plus the testlibrary
>>>>>>>> really should have a nominal owner to coordinate things - and we
>>>>>>>> need to make sure the hotspot and JDK versions are kept in sync.
>>>>>
>>>>> I guess that I and Christian usually try to monitor the changes to
>>>>> the hotspot testlibrary, and I think Igor will start do it as well
>>>>> :) This way, we at least have one from runtime, one from compiler
>>>>> and one from gc.
>>>>>
>>>>> As for keeping the JDK and HotSpot versions in sync, well that is a
>>>>> problem. One of the issues is that just because we in the hotspot
>>>>> team reviewed and approved changes to HotSpot's testlibrary does not
>>>>> automatically mean that the JDK team wants these patches. Therefore,
>>>>> keeping the JDK and the HotSpot test libraries in sync will require
>>>>> more work than just "moving patches".
>>>>>
>>>>> Unfortunately, I do not have any good solution for this at the
>>>>> moment :(
>>>>>
>>>>>>> I fully agree w/ you and I'm really sorry about the situation.
>>>>>>> But I don't see big difference between 'hotspot-runtime-dev' and
>>>>>>> 'hotspot-compiler-dev', so *this* change "really have to be
>>>>>>> reviewed more broadly as they affect everyone".
>>>>>>
>>>>>> I meant reviewing on hotspot-runtime-dev as well as
>>>>>> hotspot-compiler-dev, not in place of. hotspot-dev could have been
>>>>>> in place.
>>>>>
>>>>> I think hotspot-dev is better for changes to the hotspot testlibrary
>>>>> since the changes affect all the hotspot devevelopers.
>>>>>
>>>>> Thanks,
>>>>> Erik
>>>>>
>>>>>> Cheers,
>>>>>> David
>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Igor Ignatyev
>>>>>>>>>>>
>>>>>>>>>>> On 09/12/2013 06:38 AM, Christian Tornqvist wrote:
>>>>>>>>>>>> Hi everyone,
>>>>>>>>>>>>
>>>>>>>>>>>> Small change in JDKToolFinder so that it will now look in
>>>>>>>>>>>> compile.jdk if the tool is not found in test.jdk. I’ve tested
>>>>>>>>>>>> it locally by running tests with test.jdk set to a JRE and
>>>>>>>>>>>> compile.jdk set to JDK to see that they work correctly.
>>>>>>>>>>>>
>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ctornqvi/webrev/8014905/webrev.02
>>>>>>>>>>>> /
>>>>>>>>>>>>
>>>>>>>>>>>> Bug is unfortunately not visible externally at this time L
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Christian
>>>>>>>>>>>>
>>>>
>>