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

[Ioi Lam]

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
>>>>>>>>
>>>>>>>> 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 theycan 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


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
>>>>>>
>>>>>
>>>>
>>>
>>
>