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 <ioi.lam at oracle.com> 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 <ioi.lam at oracle.com> 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
>
>