RFR(S): 8193434: [GRAAL] Graal classes are not loaded with -Xshare:dump
Calvin Cheung
calvin.cheung at oracle.com
Thu Mar 1 21:10:48 UTC 2018
Thanks, Jiangli.
Calvin
On 3/1/18, 11:13 AM, Jiangli Zhou wrote:
> Looks good.
>
> Thanks,
> Jiangli
>
>> On Mar 1, 2018, at 9:33 AM, Calvin Cheung<calvin.cheung at oracle.com> 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