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

Wed Feb 28 20:56:30 UTC 2018

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.

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
>