RFR (XS) 8185160 -XX:DumpLoadedClassList omits graal classes

Fri Oct 20 00:02:33 UTC 2017

> On Oct 19, 2017, at 4:06 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 10/19/17 2:57 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>> 
>>> On Oct 19, 2017, at 10:55 AM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>> 
>>> 
>>> 
>>> On 10/19/17 9:47 AM, Jiangli Zhou wrote:
>>>> 
>>>>> On Oct 18, 2017, at 9:50 PM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 10/18/17 9:35 PM, Jiangli Zhou wrote:
>>>>>> Hi Ioi,
>>>>>> 
>>>>>>> On Oct 18, 2017, at 4:54 PM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 10/17/17 9:04 PM, David Holmes wrote:
>>>>>>>> Hi Ioi,
>>>>>>>> 
>>>>>>>> On 18/10/2017 1:52 PM, Ioi Lam wrote:
>>>>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v01/ <http://cr.openjdk.java.net/%7Eiklam/jdk10/8185160-dump-class-list-omits-graal.v01/> 
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8185160 <https://bugs.openjdk.java.net/browse/JDK-8185160>
>>>>>>>>> 
>>>>>>>>> Summary:
>>>>>>>>> 
>>>>>>>>> DumpLoadedClassList skips classes that can't be stored into the CDS
>>>>>>>>> archive. For boot and platform loaders, only classes from the JRT image
>>>>>>>>> are listed.
>>>>>>>>> 
>>>>>>>>> The test for "is this class from JRT image" was insufficient. It
>>>>>>>>> checked only classes whose source ends with "...../modules". For the
>>>>>>>>> platform loader, which loads the graal classes, the source is in the
>>>>>>>>> form "jrt:/jdk.internal.vm.compiler".
>>>>>>>>> 
>>>>>>>>> The fix is to also check for a "jrt:" prefix.
>>>>>>>> 
>>>>>>>> So ./share/classfile/classLoader.hpp:
>>>>>>>> 
>>>>>>>> static bool is_jrt(const char* name) { return string_ends_with(name, MODULES_IMAGE_NAME); }
>>>>>>>> 
>>>>>>>> is a badly named function or just plain broken?
>>>>>>>> 
>>>>>>> 
>>>>>>> I've renamed all the is_jrt() functions to is_modules_image(), so it's clear as to what it does.
>>>>>>> 
>>>>>>> See http://cr.openjdk.java.net/~iklam/jdk10/8185160-dump-class-list-omits-graal.v02/ <http://cr.openjdk.java.net/%7Eiklam/jdk10/8185160-dump-class-list-omits-graal.v02/>
>>>>>> 
>>>>>> The logic looks puzzling. For both NULL class loader and the PlatformClassLoader:
>>>>>> 
>>>>>> The first if check at line 5938 excludes any classes not in the jimage.
>>>>>> 
>>>>>> 5938           if (!ClassLoader::is_modules_image(stream->source()) && strncmp(stream->source(), "jrt:", 4) != 0) {
>>>>>> 5939             skip = true;
>>>>>> 5940           }
>>>>>> The second if check at line 5942 skips any classes from the boot append class path loaded by the NULL class loader. The second check skips a subset of the classes that’s already skipped by the first check. The second check is redundant.
>>>>>> 
>>>>> You're right. The second check is redundant. I'll remove it.
>>>>> 
>>>>>> 5942           if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {
>>>>>> 5943             // For the boot loader, also skip all classes that are loaded from the appended entries
>>>>>> 5944             skip = true;
>>>>>> 5945           }
>>>>>> What about the classes from -Xbootclasspath/a (not in --patch-module and --upgrade-module-path)?
>>>>>> 
>>>>> I am not sure what you mean here.
>>>> 
>>>> This excludes all classes from the -Xbootclasspath/a. However, classes from -Xbootclasspath/a can be archived and used at runtime properly. 
>>>> 
>>> 
>>> Hi Jiangli,
>>> 
>>> You're right. I misunderstood the original comments about the treatment of the bootclasspath/a classes. Here's the updated code that should be easier to understand:
>>> 
>>>         bool skip = false;
>>>         if (class_loader == NULL || SystemDictionary::is_platform_class_loader(class_loader)) {
>>>           // For the boot and platform class loaders, skip classes that are not found in the
>>>           // java runtime image, such as those found in the --patch-module entries.
>>>           // These classes can't be loaded from the archive during runtime.
>>>           if (!ClassLoader::is_modules_image(stream->source()) && strncmp(stream->source(), "jrt:", 4) != 0) {
>>>             skip = true;
>>>           }
>>> 
>>>           if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source())) {
>>>             // .. but don't skip the boot classes that are loaded from -Xbootclasspath/a
>>>             // as they can be loaded from the archive during runtime.
>>>             skip = false;
>>>           }
>>>         }
>> 
>> Can you please check with a exploded build if the above also works? What is the stream->source() from classes from exploded build loaded by the NULL loader and the PlatformClassLoader?
>> 
> 
> Hi Jiangli,
> 
> I checked the exploded build and all classes are skipped now, because they are not loaded from the $JAVA_HOME/lib/modules file. The stream->source looks like this:
> 
> skip writing class sun/security/util/FilePermCompat from source /jdk/bld/cons/jdk/modules/java.base to classlist file
> 
> However, I am not sure if this is relevant, since CDS is not supported with the exploded build.
> 
> BTW, DumpLoadedClassList is used during the JDK build process to generate the classlist file. However, this uses the interim image, which is not an exploded build:
> 
> open/make/GenerateLinkOptData.gmk:    $(FIXPATH) $(INTERIM_IMAGE_DIR)/bin/java -XX:DumpLoadedClassList=$@ \
> 
> 
> $ ./support/interim-image/bin/java -cp /jdk/tmp/jtreg/work/classes/2/runtime/AppCDS/DumpClassList.d:/jdk/cons/closed/test/hotspot/jtreg/runtime/AppCDS:/jdk/tmp/jtreg/work/classes/2/open/test/lib:/jdk/cons/open/test/lib:/java_re2/misc/promoted/jtreg/4.2/fcs/b09/binaries/jtreg/lib/javatest.jar:/jtreg/4.2/fcs/b09/binaries/jtreg/lib/jtreg.jar -XX:DumpLoadedClassList=app.list --patch-module=java.base=/jdk/tmp/jtreg/work/classes/2/runtime/AppCDS/DumpClassList.d/javabase.jar -Xbootclasspath/a:/jdk/tmp/jtreg/work/classes/2/runtime/AppCDS/DumpClassList.d/bootappend.jar -cp /jdk/tmp/jtreg/work/classes/2/runtime/AppCDS/DumpClassList.d/app.jar ArrayListTest
> hello world.
> skip writing class java/lang/NewClass from source /jdk/tmp/jtreg/work/classes/2/runtime/AppCDS/DumpClassList.d/javabase.jar to classlist file
> NewClass
> class java.lang.NewClass
> Foo
> class boot.append.Foo
> ioilinux ~/jdk/bld/cons$ wc app.list
>   701   701 22630 app.list

That’s what I expected. Thanks for verifying it. Since the goal here is to generate a more “precise” classlist for CDS and CDS does not support exploded build, it would make sense to print a warning and disable DumpLoadedClassList if ClassLoader::has_jrt_entry() is not true.

Thanks,
Jiangli

> 
> 
> Thanks
> - Ioi
> 
>> Thanks,
>> Jiangli
>> 
>>> 
>>> Thanks
>>> - Ioi
>>> 
>>> 
>>>> Thanks,
>>>> Jiangli
>>>> 
>>>>> 
>>>>> Thanks
>>>>> - Ioi
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>>>> I also restructured the code to make it easier to read -- the nesting of || and && is hard to follow.
>>>>>>>>> 
>>>>>>>>> The original code had a subtle bug with the "!" operator:
>>>>>>>>> 
>>>>>>>>> 5938   if (((class_loader == NULL && !ClassLoader::contains_append_entry(stream->source())) ||
>>>>>>>>> 5939 SystemDictionary::is_platform_class_loader(class_loader)) &&
>>>>>>>>> 5940       !ClassLoader::is_jrt(stream->source())) {
>>>>>>>>> 
>>>>>>>>> So if (class_loader == NULL && ClassLoader::contains_append_entry(stream->source()) was true,
>>>>>>>>> the class would NOT be skipped.
>>>>>>>>> 
>>>>>>>>> The new version removes the "!".
>>>>>>>> 
>>>>>>>> So there should be an updated test to trigger the original bug.
>>>>>>>> 
>>>>>>> 
>>>>>>> I added a test case, but it's in the closed repo for now (since the test case uses utilities in the closed AppCDS tests). This test will be opened as part of JDK-8185996
>>>>>>> 
>>>>>>> Thanks
>>>>>>> - Ioi
>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> - Ioi
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
> 