RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Ioi Lam
Hi,

Please review this small change in the AppCDS tests

https://bugs.openjdk.java.net/browse/JDK-8193664
http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/

Summary:

     * This patch changes tests ONLY. There's no code change.
     * The change affects ONLY the commercial JDK built by Oracle.
     * It does not affect the OpenJDK

     * Oracle commercial JDK 10 still requires -XX:+UnlockCommercialFeatures
       to be specified when using AppCDS tests. This requirement will be
       removed soon in the future, but meanwhile, we need to add a few
       -XX:+UnlockCommercialFeatures flags in the test cases to make
them pass.

Tested under both Oracle commercial JDK and OpenJDK builds. All affected
tests passed in both configurations.

Thanks
- Ioi


Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Jiangli Zhou
Looks good to me. Please also update the copyright headers before integration.

Thanks,
Jiangli

> On Jan 10, 2018, at 11:47 AM, Ioi Lam <[hidden email]> wrote:
>
> Hi,
>
> Please review this small change in the AppCDS tests
>
> https://bugs.openjdk.java.net/browse/JDK-8193664
> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/
>
> Summary:
>
>     * This patch changes tests ONLY. There's no code change.
>     * The change affects ONLY the commercial JDK built by Oracle.
>     * It does not affect the OpenJDK
>
>     * Oracle commercial JDK 10 still requires -XX:+UnlockCommercialFeatures
>       to be specified when using AppCDS tests. This requirement will be
>       removed soon in the future, but meanwhile, we need to add a few
>       -XX:+UnlockCommercialFeatures flags in the test cases to make them pass.
>
> Tested under both Oracle commercial JDK and OpenJDK builds. All affected
> tests passed in both configurations.
>
> Thanks
> - Ioi
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

David Holmes
In reply to this post by Ioi Lam
Hi Ioi,

I have two small reservations ...

1. The duplicate definitions of commercial()
2. The use of -showversion in the openJDK case. Might not this interfere
with output parsing?

Thanks,
David

On 11/01/2018 5:47 AM, Ioi Lam wrote:

> Hi,
>
> Please review this small change in the AppCDS tests
>
> https://bugs.openjdk.java.net/browse/JDK-8193664
> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>
>
> Summary:
>
>      * This patch changes tests ONLY. There's no code change.
>      * The change affects ONLY the commercial JDK built by Oracle.
>      * It does not affect the OpenJDK
>
>      * Oracle commercial JDK 10 still requires
> -XX:+UnlockCommercialFeatures
>        to be specified when using AppCDS tests. This requirement will be
>        removed soon in the future, but meanwhile, we need to add a few
>        -XX:+UnlockCommercialFeatures flags in the test cases to make
> them pass.
>
> Tested under both Oracle commercial JDK and OpenJDK builds. All affected
> tests passed in both configurations.
>
> Thanks
> - Ioi
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Mikhailo Seledtsov
In reply to this post by Ioi Lam
Changes look good,

Misha

On 1/10/18, 11:47 AM, Ioi Lam wrote:

> Hi,
>
> Please review this small change in the AppCDS tests
>
> https://bugs.openjdk.java.net/browse/JDK-8193664
> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>
>
> Summary:
>
>     * This patch changes tests ONLY. There's no code change.
>     * The change affects ONLY the commercial JDK built by Oracle.
>     * It does not affect the OpenJDK
>
>     * Oracle commercial JDK 10 still requires
> -XX:+UnlockCommercialFeatures
>       to be specified when using AppCDS tests. This requirement will be
>       removed soon in the future, but meanwhile, we need to add a few
>       -XX:+UnlockCommercialFeatures flags in the test cases to make
> them pass.
>
> Tested under both Oracle commercial JDK and OpenJDK builds. All affected
> tests passed in both configurations.
>
> Thanks
> - Ioi
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Ioi Lam
In reply to this post by David Holmes
Hi David,

Thanks for the review. I've removed the duplicated code and added a new
method TestCommon.makeCommandLineForAppCDS. The code looks like this now:

!  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
!    TestCommon.makeCommandLineForAppCDS(
        "-XX:+UseAppCDS",
        "-XX:+UseCompressedOops",
        "-XX:+UseG1GC",
        "-cp", appJar,
        "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
        "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
        "-Xshare:dump",
!      "-Xlog:cds,cds+hashtables"));

Updated webrev is at

http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/

I also updated the copyright years as mentioned by Jiangli.

Thanks
- Ioi


On 1/10/18 2:31 PM, David Holmes wrote:

> Hi Ioi,
>
> I have two small reservations ...
>
> 1. The duplicate definitions of commercial()
> 2. The use of -showversion in the openJDK case. Might not this
> interfere with output parsing?
>
> Thanks,
> David
>
> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>> Hi,
>>
>> Please review this small change in the AppCDS tests
>>
>> https://bugs.openjdk.java.net/browse/JDK-8193664
>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>
>>
>> Summary:
>>
>>      * This patch changes tests ONLY. There's no code change.
>>      * The change affects ONLY the commercial JDK built by Oracle.
>>      * It does not affect the OpenJDK
>>
>>      * Oracle commercial JDK 10 still requires
>> -XX:+UnlockCommercialFeatures
>>        to be specified when using AppCDS tests. This requirement will be
>>        removed soon in the future, but meanwhile, we need to add a few
>>        -XX:+UnlockCommercialFeatures flags in the test cases to make
>> them pass.
>>
>> Tested under both Oracle commercial JDK and OpenJDK builds. All affected
>> tests passed in both configurations.
>>
>> Thanks
>> - Ioi
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

David Holmes
Hi Ioi,

On 11/01/2018 2:42 PM, Ioi Lam wrote:

> Hi David,
>
> Thanks for the review. I've removed the duplicated code and added a new
> method TestCommon.makeCommandLineForAppCDS. The code looks like this now:
>
> !  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
> !    TestCommon.makeCommandLineForAppCDS(
>         "-XX:+UseAppCDS",
>         "-XX:+UseCompressedOops",
>         "-XX:+UseG1GC",
>         "-cp", appJar,
>         "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
>         "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
>         "-Xshare:dump",
> !      "-Xlog:cds,cds+hashtables"));

Looks good! Thanks for that.

One comment:

  public static String[] makeCommandLineForAppCDS(String... args) throws
Exception {

why the throws clause?

Thanks,
David

> Updated webrev is at
>
> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/ 
>
>
> I also updated the copyright years as mentioned by Jiangli.
>
> Thanks
> - Ioi
>
>
> On 1/10/18 2:31 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> I have two small reservations ...
>>
>> 1. The duplicate definitions of commercial()
>> 2. The use of -showversion in the openJDK case. Might not this
>> interfere with output parsing?
>>
>> Thanks,
>> David
>>
>> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>>> Hi,
>>>
>>> Please review this small change in the AppCDS tests
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8193664
>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>>
>>>
>>> Summary:
>>>
>>>      * This patch changes tests ONLY. There's no code change.
>>>      * The change affects ONLY the commercial JDK built by Oracle.
>>>      * It does not affect the OpenJDK
>>>
>>>      * Oracle commercial JDK 10 still requires
>>> -XX:+UnlockCommercialFeatures
>>>        to be specified when using AppCDS tests. This requirement will be
>>>        removed soon in the future, but meanwhile, we need to add a few
>>>        -XX:+UnlockCommercialFeatures flags in the test cases to make
>>> them pass.
>>>
>>> Tested under both Oracle commercial JDK and OpenJDK builds. All affected
>>> tests passed in both configurations.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Ioi Lam


On 1/10/18 10:04 PM, David Holmes wrote:

> Hi Ioi,
>
> On 11/01/2018 2:42 PM, Ioi Lam wrote:
>> Hi David,
>>
>> Thanks for the review. I've removed the duplicated code and added a
>> new method TestCommon.makeCommandLineForAppCDS. The code looks like
>> this now:
>>
>> !  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
>> !    TestCommon.makeCommandLineForAppCDS(
>>         "-XX:+UseAppCDS",
>>         "-XX:+UseCompressedOops",
>>         "-XX:+UseG1GC",
>>         "-cp", appJar,
>>         "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
>>         "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
>>         "-Xshare:dump",
>> !      "-Xlog:cds,cds+hashtables"));
>
> Looks good! Thanks for that.
>
> One comment:
>
>  public static String[] makeCommandLineForAppCDS(String... args)
> throws Exception {
>
> why the throws clause?
>
Oops, that was a mistake. I'll remove it.

Thanks
- Ioi

> Thanks,
> David
>
>> Updated webrev is at
>>
>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/ 
>>
>>
>> I also updated the copyright years as mentioned by Jiangli.
>>
>> Thanks
>> - Ioi
>>
>>
>> On 1/10/18 2:31 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> I have two small reservations ...
>>>
>>> 1. The duplicate definitions of commercial()
>>> 2. The use of -showversion in the openJDK case. Might not this
>>> interfere with output parsing?
>>>
>>> Thanks,
>>> David
>>>
>>> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>>>> Hi,
>>>>
>>>> Please review this small change in the AppCDS tests
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8193664
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>>>
>>>>
>>>> Summary:
>>>>
>>>>      * This patch changes tests ONLY. There's no code change.
>>>>      * The change affects ONLY the commercial JDK built by Oracle.
>>>>      * It does not affect the OpenJDK
>>>>
>>>>      * Oracle commercial JDK 10 still requires
>>>> -XX:+UnlockCommercialFeatures
>>>>        to be specified when using AppCDS tests. This requirement
>>>> will be
>>>>        removed soon in the future, but meanwhile, we need to add a few
>>>>        -XX:+UnlockCommercialFeatures flags in the test cases to
>>>> make them pass.
>>>>
>>>> Tested under both Oracle commercial JDK and OpenJDK builds. All
>>>> affected
>>>> tests passed in both configurations.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

Ioi Lam


On 1/10/18 10:10 PM, Ioi Lam wrote:

>
>
> On 1/10/18 10:04 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 11/01/2018 2:42 PM, Ioi Lam wrote:
>>> Hi David,
>>>
>>> Thanks for the review. I've removed the duplicated code and added a
>>> new method TestCommon.makeCommandLineForAppCDS. The code looks like
>>> this now:
>>>
>>> !  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
>>> !    TestCommon.makeCommandLineForAppCDS(
>>>         "-XX:+UseAppCDS",
>>>         "-XX:+UseCompressedOops",
>>>         "-XX:+UseG1GC",
>>>         "-cp", appJar,
>>>         "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
>>>         "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
>>>         "-Xshare:dump",
>>> !      "-Xlog:cds,cds+hashtables"));
>>
>> Looks good! Thanks for that.
>>
>> One comment:
>>
>>  public static String[] makeCommandLineForAppCDS(String... args)
>> throws Exception {
>>
>> why the throws clause?
>>
> Oops, that was a mistake. I'll remove it.
>

Oh, I checked again, and the throws clause is necessary because this
method calls BuildHelper.isCommercialBuild(), which throws Exception. I
think the throws clause in the latter method is really not necessary,
but that's outside of the scope of this changeset.

So after all, I will keep the changes the same as the last posted
version, and push that today.

Thanks
- Ioi

> Thanks
> - Ioi
>> Thanks,
>> David
>>
>>> Updated webrev is at
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/ 
>>>
>>>
>>> I also updated the copyright years as mentioned by Jiangli.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 1/10/18 2:31 PM, David Holmes wrote:
>>>> Hi Ioi,
>>>>
>>>> I have two small reservations ...
>>>>
>>>> 1. The duplicate definitions of commercial()
>>>> 2. The use of -showversion in the openJDK case. Might not this
>>>> interfere with output parsing?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this small change in the AppCDS tests
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8193664
>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>>>>
>>>>>
>>>>> Summary:
>>>>>
>>>>>      * This patch changes tests ONLY. There's no code change.
>>>>>      * The change affects ONLY the commercial JDK built by Oracle.
>>>>>      * It does not affect the OpenJDK
>>>>>
>>>>>      * Oracle commercial JDK 10 still requires
>>>>> -XX:+UnlockCommercialFeatures
>>>>>        to be specified when using AppCDS tests. This requirement
>>>>> will be
>>>>>        removed soon in the future, but meanwhile, we need to add a
>>>>> few
>>>>>        -XX:+UnlockCommercialFeatures flags in the test cases to
>>>>> make them pass.
>>>>>
>>>>> Tested under both Oracle commercial JDK and OpenJDK builds. All
>>>>> affected
>>>>> tests passed in both configurations.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>>>
>>>>>
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8193664 [10] - AppCDS tests should use -XX:+UnlockCommercialFeatures when running with commercial JDK

David Holmes
On 12/01/2018 10:35 AM, Ioi Lam wrote:

> On 1/10/18 10:10 PM, Ioi Lam wrote:
>> On 1/10/18 10:04 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 11/01/2018 2:42 PM, Ioi Lam wrote:
>>>> Hi David,
>>>>
>>>> Thanks for the review. I've removed the duplicated code and added a
>>>> new method TestCommon.makeCommandLineForAppCDS. The code looks like
>>>> this now:
>>>>
>>>> !  ProcessBuilder dumpPb = ProcessTools.createJavaProcessBuilder(true,
>>>> !    TestCommon.makeCommandLineForAppCDS(
>>>>         "-XX:+UseAppCDS",
>>>>         "-XX:+UseCompressedOops",
>>>>         "-XX:+UseG1GC",
>>>>         "-cp", appJar,
>>>>         "-XX:SharedArchiveConfigFile=" + sharedArchiveConfigFile,
>>>>         "-XX:SharedArchiveFile=./SharedStringsBasic.jsa",
>>>>         "-Xshare:dump",
>>>> !      "-Xlog:cds,cds+hashtables"));
>>>
>>> Looks good! Thanks for that.
>>>
>>> One comment:
>>>
>>>  public static String[] makeCommandLineForAppCDS(String... args)
>>> throws Exception {
>>>
>>> why the throws clause?
>>>
>> Oops, that was a mistake. I'll remove it.
>>
>
> Oh, I checked again, and the throws clause is necessary because this
> method calls BuildHelper.isCommercialBuild(), which throws Exception. I
> think the throws clause in the latter method is really not necessary,
> but that's outside of the scope of this changeset.

It propagates up from:

  properties.load(new FileReader(getReleaseFile()));

so seems unavoidable.

Cheers,
David

> So after all, I will keep the changes the same as the last posted
> version, and push that today.
>
> Thanks
> - Ioi
>> Thanks
>> - Ioi
>>> Thanks,
>>> David
>>>
>>>> Updated webrev is at
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v02/ 
>>>>
>>>>
>>>> I also updated the copyright years as mentioned by Jiangli.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>> On 1/10/18 2:31 PM, David Holmes wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> I have two small reservations ...
>>>>>
>>>>> 1. The duplicate definitions of commercial()
>>>>> 2. The use of -showversion in the openJDK case. Might not this
>>>>> interfere with output parsing?
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 11/01/2018 5:47 AM, Ioi Lam wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this small change in the AppCDS tests
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8193664
>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8193664-commercial-appcds-testbug.v01/ 
>>>>>>
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>>      * This patch changes tests ONLY. There's no code change.
>>>>>>      * The change affects ONLY the commercial JDK built by Oracle.
>>>>>>      * It does not affect the OpenJDK
>>>>>>
>>>>>>      * Oracle commercial JDK 10 still requires
>>>>>> -XX:+UnlockCommercialFeatures
>>>>>>        to be specified when using AppCDS tests. This requirement
>>>>>> will be
>>>>>>        removed soon in the future, but meanwhile, we need to add a
>>>>>> few
>>>>>>        -XX:+UnlockCommercialFeatures flags in the test cases to
>>>>>> make them pass.
>>>>>>
>>>>>> Tested under both Oracle commercial JDK and OpenJDK builds. All
>>>>>> affected
>>>>>> tests passed in both configurations.
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>
>>
>