RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

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

RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Ioi Lam
Hi,

Please review this change to support AppCDS on all 64-bit Linux
platforms as well as AIX.

     https://bugs.openjdk.java.net/browse/JDK-8191927
http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/

The patch must be applied on top of the AppCDS patch:

http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/

The patch was initially contributed by Volker Simonis. I modified it a
little to avoid repeating the platform enumerations in all the test
cases. Instead, I added a new test property, so the tests that require
AppCDS support for custom loaders can be written as:

     * @requires vm.cds.custom.loaders

I've tested on Linux manually, and I am now running on all of the
Oracle-supported platforms using our internal test harness.

Thanks
- Ioi
Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Volker Simonis
Hi Ioi,

the change and your refactorings to it look good :) I specially like
the introduction of  "@requires vm.cds.custom.loaders" to simplify the
logic.

I've only found two little problems:

- the patch for the test
"cacheObject/CheckCachedResolvedReferences.java" is empty. That's
probably because it is not related to AppCDS. But the test can and
should still run on all 64-bit Linux and AIX platfroms as well.

- the following two tests still get executed in the product build
where they fail because they use non-product flags:
runtime/appcds/DumpClassList.java
runtime/appcds/ProhibitedPackage.java

I think you fixed that in one of your previous patches but it probably
got lost again before pushing.

The attached "addon.patch" fixes these two problems.

Thank you and bet regards,
Volker


On Mon, Nov 27, 2017 at 10:42 PM, Ioi Lam <[hidden email]> wrote:

> Hi,
>
> Please review this change to support AppCDS on all 64-bit Linux platforms as
> well as AIX.
>
>     https://bugs.openjdk.java.net/browse/JDK-8191927
> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/
>
> The patch must be applied on top of the AppCDS patch:
>
> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>
> The patch was initially contributed by Volker Simonis. I modified it a
> little to avoid repeating the platform enumerations in all the test cases.
> Instead, I added a new test property, so the tests that require AppCDS
> support for custom loaders can be written as:
>
>     * @requires vm.cds.custom.loaders
>
> I've tested on Linux manually, and I am now running on all of the
> Oracle-supported platforms using our internal test harness.
>
> Thanks
> - Ioi

addon.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Ioi Lam
Hi Volker,

Thanks for the review


On 11/28/17 8:46 AM, Volker Simonis wrote:

> Hi Ioi,
>
> the change and your refactorings to it look good :) I specially like
> the introduction of  "@requires vm.cds.custom.loaders" to simplify the
> logic.
>
> I've only found two little problems:
>
> - the patch for the test
> "cacheObject/CheckCachedResolvedReferences.java" is empty. That's
> probably because it is not related to AppCDS. But the test can and
> should still run on all 64-bit Linux and AIX platfroms as well.

I've fixed this. I thought it's unrelated to custom loaders, but it actually
does. I've added a comment to reflect that:


--- a/CheckCachedResolvedReferences.java    Tue Nov 28 21:04:42 2017 +0530
+++ b/CheckCachedResolvedReferences.java    Tue Nov 28 09:33:46 2017 -0800
@@ -26,8 +26,7 @@
   * @test
   * @summary Test resolved_references
   * @requires (vm.opt.UseCompressedOops == null) |
(vm.opt.UseCompressedOops == true)
- * @requires (sun.arch.data.model == "64")
- * @requires ((os.family == "linux") & (os.arch=="amd64")) | (os.family
== "solaris")
+ * @requires vm.cds.custom.loaders
   * @requires (vm.gc=="null")
   * @library /test/lib /test/hotspot/jtreg/runtime/appcds
   * @modules java.base/jdk.internal.misc
@@ -53,9 +52,9 @@
          String helloJarPath = ClassFileInstaller.getJarPath("hello.jar");

          String classlist[] = new String[] {
-            "CheckCachedResolvedReferencesApp",
-            "java/lang/Object id: 1",
-            "Hello id: 2 super: 1 source: " + helloJarPath
+            "CheckCachedResolvedReferencesApp",            // built-in
app loader
+            "java/lang/Object id: 1",                      // boot loader
+            "Hello id: 2 super: 1 source: " + helloJarPath // custom loader
          };

          TestCommon.testDump(appJar, classlist, use_whitebox_jar);



> - the following two tests still get executed in the product build
> where they fail because they use non-product flags:
> runtime/appcds/DumpClassList.java
> runtime/appcds/ProhibitedPackage.java
>
> I think you fixed that in one of your previous patches but it probably
> got lost again before pushing.

I wanted to fix them in a separate changeset, so that the "move appcds
to open"
changeset is a more verbatim move. It's the following bug, and I will
post RFR shortly.

https://bugs.openjdk.java.net/browse/JDK-8191747
"[TESTBUG] runtime/AppCDS/DumpClassList.java and ProhibitedPackage.java
fail on product bits"

Instead of "@require vm.debug", I'll try to rewrite the test to remove the
dependency on -XX:+PrintSystemDictionaryAtExit

Thanks
- Ioi

> The attached "addon.patch" fixes these two problems.
>
> Thank you and bet regards,
> Volker
>
>
> On Mon, Nov 27, 2017 at 10:42 PM, Ioi Lam <[hidden email]> wrote:
>> Hi,
>>
>> Please review this change to support AppCDS on all 64-bit Linux platforms as
>> well as AIX.
>>
>>      https://bugs.openjdk.java.net/browse/JDK-8191927
>> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/
>>
>> The patch must be applied on top of the AppCDS patch:
>>
>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>>
>> The patch was initially contributed by Volker Simonis. I modified it a
>> little to avoid repeating the platform enumerations in all the test cases.
>> Instead, I added a new test property, so the tests that require AppCDS
>> support for custom loaders can be written as:
>>
>>      * @requires vm.cds.custom.loaders
>>
>> I've tested on Linux manually, and I am now running on all of the
>> Oracle-supported platforms using our internal test harness.
>>
>> Thanks
>> - Ioi

Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Volker Simonis
On Tue, Nov 28, 2017 at 6:41 PM, Ioi Lam <[hidden email]> wrote:

> Hi Volker,
>
> Thanks for the review
>
>
> On 11/28/17 8:46 AM, Volker Simonis wrote:
>>
>> Hi Ioi,
>>
>> the change and your refactorings to it look good :) I specially like
>> the introduction of  "@requires vm.cds.custom.loaders" to simplify the
>> logic.
>>
>> I've only found two little problems:
>>
>> - the patch for the test
>> "cacheObject/CheckCachedResolvedReferences.java" is empty. That's
>> probably because it is not related to AppCDS. But the test can and
>> should still run on all 64-bit Linux and AIX platfroms as well.
>
>
> I've fixed this. I thought it's unrelated to custom loaders, but it actually
> does. I've added a comment to reflect that:
>
>
> --- a/CheckCachedResolvedReferences.java    Tue Nov 28 21:04:42 2017 +0530
> +++ b/CheckCachedResolvedReferences.java    Tue Nov 28 09:33:46 2017 -0800
> @@ -26,8 +26,7 @@
>   * @test
>   * @summary Test resolved_references
>   * @requires (vm.opt.UseCompressedOops == null) | (vm.opt.UseCompressedOops
> == true)
> - * @requires (sun.arch.data.model == "64")
> - * @requires ((os.family == "linux") & (os.arch=="amd64")) | (os.family ==
> "solaris")
> + * @requires vm.cds.custom.loaders
>   * @requires (vm.gc=="null")
>   * @library /test/lib /test/hotspot/jtreg/runtime/appcds
>   * @modules java.base/jdk.internal.misc
> @@ -53,9 +52,9 @@
>          String helloJarPath = ClassFileInstaller.getJarPath("hello.jar");
>
>          String classlist[] = new String[] {
> -            "CheckCachedResolvedReferencesApp",
> -            "java/lang/Object id: 1",
> -            "Hello id: 2 super: 1 source: " + helloJarPath
> +            "CheckCachedResolvedReferencesApp",            // built-in app
> loader
> +            "java/lang/Object id: 1",                      // boot loader
> +            "Hello id: 2 super: 1 source: " + helloJarPath // custom loader
>          };
>
>          TestCommon.testDump(appJar, classlist, use_whitebox_jar);
>

OK

>
>
>> - the following two tests still get executed in the product build
>> where they fail because they use non-product flags:
>> runtime/appcds/DumpClassList.java
>> runtime/appcds/ProhibitedPackage.java
>>
>> I think you fixed that in one of your previous patches but it probably
>> got lost again before pushing.
>
>
> I wanted to fix them in a separate changeset, so that the "move appcds to
> open"
> changeset is a more verbatim move. It's the following bug, and I will post
> RFR shortly.
>
> https://bugs.openjdk.java.net/browse/JDK-8191747
> "[TESTBUG] runtime/AppCDS/DumpClassList.java and ProhibitedPackage.java fail
> on product bits"
>
> Instead of "@require vm.debug", I'll try to rewrite the test to remove the
> dependency on -XX:+PrintSystemDictionaryAtExit
>

OK, that's fine for me.

Thumbs up then for the change!

Volker

> Thanks
> - Ioi
>
>
>> The attached "addon.patch" fixes these two problems.
>>
>> Thank you and bet regards,
>> Volker
>>
>>
>> On Mon, Nov 27, 2017 at 10:42 PM, Ioi Lam <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> Please review this change to support AppCDS on all 64-bit Linux platforms
>>> as
>>> well as AIX.
>>>
>>>      https://bugs.openjdk.java.net/browse/JDK-8191927
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/
>>>
>>> The patch must be applied on top of the AppCDS patch:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>>>
>>> The patch was initially contributed by Volker Simonis. I modified it a
>>> little to avoid repeating the platform enumerations in all the test
>>> cases.
>>> Instead, I added a new test property, so the tests that require AppCDS
>>> support for custom loaders can be written as:
>>>
>>>      * @requires vm.cds.custom.loaders
>>>
>>> I've tested on Linux manually, and I am now running on all of the
>>> Oracle-supported platforms using our internal test harness.
>>>
>>> Thanks
>>> - Ioi
>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Mikhailo Seledtsov
In reply to this post by Ioi Lam
Hi Ioi,

   Overall the change looks good. I have just one comment:

1. In test/lib/Platform.java you note in the comment that the condition
should match one in ClassListParser::load_class_from_source(). However,
there is not reverse reference. That is, there is no note/comment in
ClassListParser::load_class_from_source() to update
test.lib.Platform.areCustomLoadersSupportedForCDS() if the condition
changes in load_class_from_source().

I can think of couple of ways to address this:

A. Simple quick way, just add the comment in classListParser.cpp that
test library should be updated if this condition is updated.

B. Add a white box API similar to WhiteBox.isCDSIncludedInVmBuild(); and
have a method in VM that would compute the result based on the platform
conditionals.

The option B is more robust in my view, but I will not have strong
objection to option A either. Also, I understand that option B can
result in some sort of complex dependencies.


Thank you,

Misha




On 11/27/2017 01:42 PM, Ioi Lam wrote:

> Hi,
>
> Please review this change to support AppCDS on all 64-bit Linux
> platforms as well as AIX.
>
>     https://bugs.openjdk.java.net/browse/JDK-8191927
> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/ 
>
>
> The patch must be applied on top of the AppCDS patch:
>
> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>
> The patch was initially contributed by Volker Simonis. I modified it a
> little to avoid repeating the platform enumerations in all the test
> cases. Instead, I added a new test property, so the tests that require
> AppCDS support for custom loaders can be written as:
>
>     * @requires vm.cds.custom.loaders
>
> I've tested on Linux manually, and I am now running on all of the
> Oracle-supported platforms using our internal test harness.
>
> Thanks
> - Ioi

Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Ioi Lam


On 11/29/17 2:15 PM, mikhailo wrote:

> Hi Ioi,
>
>   Overall the change looks good. I have just one comment:
>
> 1. In test/lib/Platform.java you note in the comment that the
> condition should match one in
> ClassListParser::load_class_from_source(). However, there is not
> reverse reference. That is, there is no note/comment in
> ClassListParser::load_class_from_source() to update
> test.lib.Platform.areCustomLoadersSupportedForCDS() if the condition
> changes in load_class_from_source().
>
> I can think of couple of ways to address this:
>
> A. Simple quick way, just add the comment in classListParser.cpp that
> test library should be updated if this condition is updated.
>
> B. Add a white box API similar to WhiteBox.isCDSIncludedInVmBuild();
> and have a method in VM that would compute the result based on the
> platform conditionals.
>
> The option B is more robust in my view, but I will not have strong
> objection to option A either. Also, I understand that option B can
> result in some sort of complex dependencies.
>

Hi Misha,

Option B will require that the VM has -XX:+WhiteBoxAPI and the
whitebox.jar in the classpath, but the Platform class can be used
without these conditions. So I don't think B is doable. I'll add the
comments as you suggested in A.

Thanks
- Ioi

>
> Thank you,
>
> Misha
>
>
>
>
> On 11/27/2017 01:42 PM, Ioi Lam wrote:
>> Hi,
>>
>> Please review this change to support AppCDS on all 64-bit Linux
>> platforms as well as AIX.
>>
>>     https://bugs.openjdk.java.net/browse/JDK-8191927
>> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/ 
>>
>>
>> The patch must be applied on top of the AppCDS patch:
>>
>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>>
>> The patch was initially contributed by Volker Simonis. I modified it
>> a little to avoid repeating the platform enumerations in all the test
>> cases. Instead, I added a new test property, so the tests that
>> require AppCDS support for custom loaders can be written as:
>>
>>     * @requires vm.cds.custom.loaders
>>
>> I've tested on Linux manually, and I am now running on all of the
>> Oracle-supported platforms using our internal test harness.
>>
>> Thanks
>> - Ioi
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR[S] 8191927 Enable AppCDS for custom loaders on all 64-bit Linux and AIX

Mikhailo Seledtsov
Thank you,

Consider it reviewed for my part; no need to post a new webrev for the
added comment.


Misha


On 11/29/2017 03:29 PM, Ioi Lam wrote:

>
>
> On 11/29/17 2:15 PM, mikhailo wrote:
>> Hi Ioi,
>>
>>   Overall the change looks good. I have just one comment:
>>
>> 1. In test/lib/Platform.java you note in the comment that the
>> condition should match one in
>> ClassListParser::load_class_from_source(). However, there is not
>> reverse reference. That is, there is no note/comment in
>> ClassListParser::load_class_from_source() to update
>> test.lib.Platform.areCustomLoadersSupportedForCDS() if the condition
>> changes in load_class_from_source().
>>
>> I can think of couple of ways to address this:
>>
>> A. Simple quick way, just add the comment in classListParser.cpp that
>> test library should be updated if this condition is updated.
>>
>> B. Add a white box API similar to WhiteBox.isCDSIncludedInVmBuild();
>> and have a method in VM that would compute the result based on the
>> platform conditionals.
>>
>> The option B is more robust in my view, but I will not have strong
>> objection to option A either. Also, I understand that option B can
>> result in some sort of complex dependencies.
>>
>
> Hi Misha,
>
> Option B will require that the VM has -XX:+WhiteBoxAPI and the
> whitebox.jar in the classpath, but the Platform class can be used
> without these conditions. So I don't think B is doable. I'll add the
> comments as you suggested in A.
>
> Thanks
> - Ioi
>
>>
>> Thank you,
>>
>> Misha
>>
>>
>>
>>
>> On 11/27/2017 01:42 PM, Ioi Lam wrote:
>>> Hi,
>>>
>>> Please review this change to support AppCDS on all 64-bit Linux
>>> platforms as well as AIX.
>>>
>>>     https://bugs.openjdk.java.net/browse/JDK-8191927
>>> http://cr.openjdk.java.net/~iklam/jdk10/8191927-appcds-custom-loader-for-more-platforms.v01/ 
>>>
>>>
>>> The patch must be applied on top of the AppCDS patch:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk10/8188791-open-appcds.v03/
>>>
>>> The patch was initially contributed by Volker Simonis. I modified it
>>> a little to avoid repeating the platform enumerations in all the
>>> test cases. Instead, I added a new test property, so the tests that
>>> require AppCDS support for custom loaders can be written as:
>>>
>>>     * @requires vm.cds.custom.loaders
>>>
>>> I've tested on Linux manually, and I am now running on all of the
>>> Oracle-supported platforms using our internal test harness.
>>>
>>> Thanks
>>> - Ioi
>>
>