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

[Ioi Lam]

On 10/19/17 5:02 PM, Jiangli Zhou wrote:
>
>> On Oct 19, 2017, at 4:06 PM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto: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
>>>>>>>>>>
>>>>>>>>>> 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
>
>
> 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.
>

Hi Jiangli,

Thanks for the suggestion. I've added the warning as you suggested:


     if (DumpLoadedClassList != NULL && stream->source() != NULL && 
classlist_file->is_open()) {
!     if (!ClassLoader::has_jrt_entry()) {
!       warning("DumpLoadedClassList and CDS are not supported in 
exploded build");
!       DumpLoadedClassList = NULL;
!     } else if 
(SystemDictionaryShared::is_sharing_possible(_loader_data) &&
           _host_klass == NULL) {
         // Only dump the classes that can be stored into CDS archive.
         // Anonymous classes such as generated LambdaForm classes are 
also not included.
         oop class_loader = _loader_data->class_loader();
         ResourceMark rm(THREAD);
         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;
           }
         }
         if (skip) {
           tty->print_cr("skip writing class %s from source %s to 
classlist file",
             _class_name->as_C_string(), stream->source());
         } else {
           classlist_file->print_cr("%s", _class_name->as_C_string());
           classlist_file->flush();
         }
       }
     }


This message is printed at run time for the exploded build:

Java HotSpot(TM) 64-Bit Server VM warning: DumpLoadedClassList and CDS 
are not supported in exploded build

What do you think?

- Ioi