RFR(S): 8193434: [GRAAL] Graal classes are not loaded with -Xshare:dump

Vladimir Kozlov vladimir.kozlov at oracle.com
Thu Mar 1 20:39:10 UTC 2018 

This is good!

I tested patch with eager JVMCI initialization and don't see warnings anymore.

With only CDS I got: Number of classes 1629

And with AppCDS: Number of classes 2570

As expected! Thanks!

Vladimir

On 3/1/18 9:33 AM, Calvin Cheung wrote:
> 
> 
> On 2/28/18, 12:56 PM, Ioi Lam wrote:
>>
>>
>>
>> On 2/28/18 10:42 AM, Jiangli Zhou wrote:
>>> I vote for this solution. Although it is not perfect since classes not in the combined dictionary 
>>> will still copied into the shared metaspaces, it is much cleaner than the conditional check in 
>>> SystemDictionary::resolve_from_stream(). 
>>
>> We can change CollectClassesClosure to collect only boot classes if UseAppCDS is false. That way, 
>> non-boot classes will not be copied into the shared metaspaces.
> I've made this additional change.
> Updated webrev:
>      http://cr.openjdk.java.net/~ccheung/8193434/webrev.03/
> 
> thanks,
> Calvin
>>
>>
>> class CollectClassesClosure : public KlassClosure {
>>   void do_klass(Klass* k) {
>>     if (!(k->is_instance_klass() && InstanceKlass::cast(k)->is_in_error_state())) {
>>       _global_klass_objects->append_if_missing(k);
>>     }
>>     if (k->is_array_klass()) {
>>       // Add in the array classes too
>>       ArrayKlass* ak = ArrayKlass::cast(k);
>>       Klass* h = ak->higher_dimension();
>>       if (h != NULL) {
>>         h->array_klasses_do(collect_array_classes);
>>       }
>>     }
>>   }
>> };
>>
>> Thanks
>> - Ioi
>>> Removing the CDS specific if block in SystemDictionary::resolve_from_stream() makes normal code 
>>> path cleaner.
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>> On Feb 28, 2018, at 7:59 AM, Ioi Lam <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>>>
>>>>
>>>>
>>>> On 2/27/18 5:38 PM, Calvin Cheung wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> Thanks for taking a look.
>>>>>
>>>>> On 2/27/18, 4:30 PM, Ioi Lam wrote:
>>>>>> Hi Calvin,
>>>>>>
>>>>>> I think the CNFE is caused by this code (in the "before" version):
>>>>>>
>>>>>> 1054 InstanceKlass* SystemDictionary::resolve_from_stream(Symbol* class_name,
>>>>>> 1055 Handle class_loader,
>>>>>> 1056 Handle protection_domain,
>>>>>> 1057 ClassFileStream* st,
>>>>>> 1058 TRAPS) {
>>>>>> 1059 #if INCLUDE_CDS
>>>>>> 1060   ResourceMark rm(THREAD);
>>>>>> 1061   if (DumpSharedSpaces && !class_loader.is_null() &&
>>>>>> 1062       !UseAppCDS && strcmp(class_name->as_C_string(), "Unnamed") != 0) {
>>>>>> 1063     // If AppCDS is not enabled, don't define the class at dump time (except for the 
>>>>>> "Unnamed"
>>>>>> 1064     // class, which is used by MethodHandles).
>>>>>> 1065 THROW_MSG_NULL(vmSymbols::java_lang_ClassNotFoundException(), class_name->as_C_string());
>>>>>> 1066   }
>>>>>> 1067 #endif
>>>>> Yes, I wrote a comment in the bug report yesterday.
>>>>>>
>>>>>> As far as I can tell, the intention is -- if UseAppCDS is not enabled, load classes only for 
>>>>>> the boot loader.
>>>>> Right.
>>>>>>
>>>>>> However, when -XX:+UseJVMCICompiler is enabled in the command-line during dumping, we get this:
>>>>>>
>>>>>> $ java -Xshare:dump -Xlog:cds,cds+hashtables -XX:+UnlockDiagnosticVMOptions 
>>>>>> -XX:SharedArchiveFile=./test.jsa -Xbatch -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI 
>>>>>> -XX:+UseJVMCICompiler
>>>>>>
>>>>>> java.lang.ClassNotFoundException: org/graalvm/compiler/hotspot/HotSpotGraalJVMCIServiceLocator
>>>>>> at java.lang.ClassLoader.defineClass2(java.base at 11-internal/Native Method)
>>>>>> at java.lang.ClassLoader.defineClass(java.base at 11-internal/ClassLoader.java:1086)
>>>>>> at java.security.SecureClassLoader.defineClass(java.base at 11-internal/SecureClassLoader.java:206)
>>>>>> at 
>>>>>> jdk.internal.loader.BuiltinClassLoader.defineClass(java.base at 11-internal/BuiltinClassLoader.java:760) 
>>>>>>
>>>>>> at 
>>>>>> jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(java.base at 11-internal/BuiltinClassLoader.java:681) 
>>>>>>
>>>>>> at 
>>>>>> jdk.internal.loader.BuiltinClassLoader.findClass(java.base at 11-internal/BuiltinClassLoader.java:562) 
>>>>>>
>>>>>> at java.lang.ClassLoader.loadClass(java.base at 11-internal/ClassLoader.java:611)
>>>>>> at java.lang.Class.forName(java.base at 11-internal/Class.java:450)
>>>>>> ...
>>>>>>
>>>>>>
>>>>>> With "-XX+UseAppCDS -Xlog:class+load=debug", we can see this class is loaded by the platform 
>>>>>> loader:
>>>>>>
>>>>>>
>>>>>> [0.325s][info ][class,load] org.graalvm.compiler.hotspot.HotSpotGraalJVMCIServiceLocator 
>>>>>> source: jrt:/jdk.internal.vm.compiler
>>>>>> [0.325s][debug][class,load]  klass: 0x00000008c0083c30 super: 0x00000008c00734c8 loader: 
>>>>>> [class loader 0x00007f30344e5870 a 
>>>>>> 'jdk/internal/loader/ClassLoaders$PlatformClassLoader'{0x000000041105cbf0}] bytes: 1443 
>>>>>> checksum: 8dae8ade
>>>>>>
>>>>>>
>>>>>> I think instead of using the _force_init_jvmci_runtime, which allows these classes to be 
>>>>>> loaded only during a portion of the dump time, a better fix would be to remove the above "if" 
>>>>>> block altogether. I think if UseAppCDS is not true, 
>>>>>> SystemDictionary::combine_shared_dictionaries() won't be called, so any non-boot classes will 
>>>>>> be left out from the CDS archive, which is what you want.
>>>>> I tried setting breakpoint in SystemDictionary::combine_shared_dictionaries() and the debugger 
>>>>> stopped there without enabling UseAppCDS.
>>>>
>>>> How about adding a check like this:
>>>>
>>>> void SystemDictionary::combine_shared_dictionaries() {
>>>>   assert(DumpSharedSpaces, "dump time only");
>>>>   if (UseAppCDS) {
>>>>     Dictionary* master_dictionary = ClassLoaderData::the_null_class_loader_data()->dictionary();
>>>>     CombineDictionariesClosure cdc(master_dictionary);
>>>>     ClassLoaderDataGraph::cld_do(&cdc);
>>>>   }
>>>>
>>>>
>>>>> My current proposed change should have the least impact on CDS (without enabling UseAppCDS).
>>>>> For getting rid of those "Preload Warning" noted by Vladimir, I think I can leave the 
>>>>> _force_init_jvmci_runtime setting; i.e. not resetting it to false at the end of force JVMCI 
>>>>> runtime init. This is what I had in my first version:
>>>>> http://cr.openjdk.java.net/~ccheung/8193434/webrev.00/ 
>>>>> <http://cr.openjdk.java.net/%7Eccheung/8193434/webrev.00/>
>>>> I think the use of _force_init_jvmci_runtime feels like an ad-hoc solution. The existing code is 
>>>> already hard to understand:
>>>>
>>>> 1061   if (DumpSharedSpaces && !class_loader.is_null() &&
>>>> 1062       !UseAppCDS && strcmp(class_name->as_C_string(), "Unnamed") != 0) {
>>>>
>>>> Our problem here is to make sure that the Java-based class loaders runs correctly during dump 
>>>> time. In the future, if more non-boot classes are loaded when running the Java-based class 
>>>> loaders, this "if" condition will get more and more complicated.
>>>>
>>>> I think it's much cleaner to modify combine_shared_dictionaries(). That way we can ensure that 
>>>> the Java-based loaders can run correctly, and also make sure that only boot classes are archived 
>>>> when UseAppCDS is false.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>>>
>>>>>> What do you think?
>>>>> I think the current fix with the "Preload Warning" is ok; it means 170 classes won't be in the 
>>>>> archive.
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/27/18 11:01 AM, Calvin Cheung wrote:
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8193434
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8193434/webrev.01/ 
>>>>>>> <http://cr.openjdk.java.net/%7Eccheung/8193434/webrev.01/>
>>>>>>>
>>>>>>> java -Xshare:dump -Xlog:cds,cds+hashtables -XX:+UnlockDiagnosticVMOptions 
>>>>>>> -XX:SharedArchiveFile=./test.jsa -Xbatch -XX:+UnlockExperimentalVMOptions -XX:+EnableJVMCI 
>>>>>>> -XX:+UseJVMCICompiler -Djvmci.Compiler=graal
>>>>>>>
>>>>>>> The above command causes the CNFE to be thrown in SystemDictionary::resolve_from_stream() 
>>>>>>> during CDS dump time.
>>>>>>> The proposed change is to add a _force_init_jvmci_runtime flag which is set at the beginning 
>>>>>>> of JVMCIRuntime::force_initialization() and reset at the end of the initialization. The flag 
>>>>>>> will be checked in SystemDictionary::resolve_from_stream() so that it will not throw CNFE 
>>>>>>> during the eager initialization of JVMCI runtime during vm init.
>>>>>>>
>>>>>>> The proposed change passed hs-tier1 through hs-tier3 testing on linux-x64, windows-x64, 
>>>>>>> sparcv9, and Mac OS X.
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>
>>

More information about the hotspot-runtime-dev mailing list