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

[Calvin Cheung]

On 2/28/18, 7:59 AM, Ioi Lam 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);
>   }
>
The above works and it will find more classes during dumping. As a 
result, I needed to adjust the UseAppCDS.java test a little bit.
Here's an update webrev:
     http://cr.openjdk.java.net/~ccheung/8193434/webrev.02/

It passed all CDS and AppCDS tests locally on linux-x64. I will run more 
testing using our test harness.

thanks,
Calvin
>
>> 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/
> 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/
>>>>
>>>> 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
>>>>
>>>
>