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

Ioi Lam ioi.lam at oracle.com
Thu Mar 1 18:34:54 UTC 2018 

Looks good. Thanks Calvin!

- Ioi


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