RFR(S): 8193434: [GRAAL] Graal classes are not loaded with -Xshare:dump
Calvin Cheung
calvin.cheung at oracle.com
Thu Mar 1 21:08:43 UTC 2018
Hi Vladimir,
Thanks for trying the patch again.
Calvin
On 3/1/18, 12:39 PM, Vladimir Kozlov wrote:
> 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