RFR(S): 8232081: Try to link all classes during dynamic CDS dump

Calvin Cheung calvin.cheung at oracle.com
Thu Mar 5 19:48:49 UTC 2020 

On 3/5/20 9:17 AM, Ioi Lam wrote:
> Hi Calvin,
>
> Looks good overall. I just have two comment:
>
> I think this is not needed:
>
>  306 void ConstantPool::resolve_class_constants(TRAPS) {
>  307   Arguments::assert_is_dumping_archive();
I've reverted this change.
>
> because this function is used to resolve all Strings in the statically 
> dumped classes to archive all the Strings. However, the archive heap 
> is not supported for the dynamic archive. So I think it's better to 
> avoid calling this function from LinkSharedClassesClosure for dynamic 
> dump.
Now, the function will not be called with dynamic dump:
1714         if (DumpSharedSpaces) {
1715           // The following function is used to resolve all Strings 
in the statically
1716           // dumped classes to archive all the Strings. The archive 
heap is not supported
1717           // for the dynamic archive.
1718 ik->constants()->resolve_class_constants(THREAD);
1719         }
>
> LinkSharedClassesClosure::_is_static -- maybe we can avoid adding this 
> field and just check "if (DumpSharedSpaces)" instead?

This is a good simplification.

btw, you've removed loader_type() from instanceKlass.hpp when you pushed 
the change for JDK-8240244. I've added it back as shared_loader_type().

updated webrev:

     http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.04/

thanks,

Calvin

>
> Thanks
> - Ioi
>
>
>
>
>
> On 3/4/20 9:13 PM, Calvin Cheung wrote:
>> Hi David, Ioi,
>>
>> Here's an updated webrev which doesn't involve changing 
>> InstanceKlass::verify_on() and made use of the changes for JDK-8240481.
>>
>>     http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.03/
>>
>> thanks,
>> Calvin
>>
>> On 3/1/20 10:14 PM, David Holmes wrote:
>>> Hi Calvin,
>>>
>>> On 28/02/2020 4:12 pm, Calvin Cheung wrote:
>>>> Hi David,
>>>>
>>>> On 2/27/20 5:40 PM, David Holmes wrote:
>>>>> Hi Calvin, Ioi,
>>>>>
>>>>> Looking good - comments below.
>>>>>
>>>>> A meta-question: normal application classes are rarely loaded but 
>>>>> not linked so I'm a little surprised this is an issue. What is the 
>>>>> main source of such classes - generated classes like lambda forms? 
>>>>> Also why do we need to link them when loading from the archive if 
>>>>> they were never linked when the application actually executed ??
>>>>
>>>> I saw that Ioi already answered the above.
>>>>
>>>> I'll try to answer your questions inline below..
>>>
>>> Responses inline below ...
>>>
>>>>>
>>>>> On 28/02/2020 10:48 am, Calvin Cheung wrote:
>>>>>> Hi David, Ioi,
>>>>>>
>>>>>> Thanks for your review and suggestions.
>>>>>>
>>>>>> On 2/26/20 9:46 PM, Ioi Lam wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/26/20 7:50 PM, David Holmes wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>> Adding core-libs-dev as you are messing with their code :)
>>>>>>>>
>>>>>>>> On 27/02/2020 1:19 pm, Calvin Cheung wrote:
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8232081
>>>>>>>>> webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/
>>>>>>>>>
>>>>>>>>> The proposed changeset for this RFE adds a 
>>>>>>>>> JVM_LinkClassesForCDS() function to be called from 
>>>>>>>>> java/lang/Shutdown to notify the JVM to link the classes 
>>>>>>>>> loaded by the builtin class loaders. The 
>>>>>>>>
>>>>>>>> This would be much less disruptive if this was handled purely 
>>>>>>>> on the VM side once we have started shutdown. No need to make 
>>>>>>>> any changes to the Java side then, nor jvm.cpp.
>>>>>>>>
>>>>>>>
>>>>>>> Hi David,
>>>>>>>
>>>>>>> To link the classes, we need to be able to run Java code -- when 
>>>>>>> linking a class X loaded by the app loader, X will be verified. 
>>>>>>> During verification of X, we may need to load additional classes 
>>>>>>> from the app loader, which executes Java code during its class 
>>>>>>> loading operations.
>>>>>>>
>>>>>>> We also need to handle all the exit conditions. As far as I can 
>>>>>>> tell, an app can exit the JVM in two ways:
>>>>>>>
>>>>>>> (1) Explicitly calling System.exit(). This will call 
>>>>>>> java.lang.Shutdown.exit() which does this:
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l163 
>>>>>>>
>>>>>>>
>>>>>>>             beforeHalt(); // native
>>>>>>>             runHooks();
>>>>>>>             halt(status);
>>>>>>>
>>>>>>> (2) When all non-daemon threads have died (e.g., falling out of 
>>>>>>> the bottom of HelloWorld.main()). There's no explicit call to 
>>>>>>> System.exit(), but the JVM will proactively call 
>>>>>>> java.lang.Shutdown.shutdown() inside 
>>>>>>> JavaThread::invoke_shutdown_hooks()
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/runtime/thread.cpp#l4331 
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l184 
>>>>>>>
>>>>>>>
>>>>>>> If we want to avoid modifying the Java code, I think we can 
>>>>>>> intercept JVM_BeforeHalt() and 
>>>>>>> JavaThread::invoke_shutdown_hooks(). This way we should be able 
>>>>>>> to handle all the classes (except those that are loaded inside 
>>>>>>> Shutdown.runHooks).
>>>>>>
>>>>>> I've implemented the above. No changes to the Java side.
>>>>>>
>>>>>> updated webrev:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.01/
>>>>>
>>>>> This looks much better to me. I wasn't sure if you were that 
>>>>> concerned about catching the classes loaded by the Shutdown hooks, 
>>>>> but it is good you are not as it seems problematic to me to 
>>>>> trigger class loading etc after the shutdown hooks have executed - 
>>>>> you may hit unexpected conditions. So this approach is good.
>>>>>
>>>>> A few minor comments:
>>>>>
>>>>> src/hotspot/share/memory/metaspaceShared.cpp
>>>>> src/hotspot/share/memory/metaspaceShared.hpp
>>>>>
>>>>> Why the change from TRAPS to "Thread* THREAD"?
>>>> I forgot why I changed it but I've changed it back and it still works.
>>>
>>> Ok.
>>>
>>>>>
>>>>> ---
>>>>>
>>>>> src/hotspot/share/oops/instanceKlass.cpp
>>>>>
>>>>> I'm not clear how verify_on is used so am unsure how the 
>>>>> additional error state check may affect code unrelated to dynamic 
>>>>> dumping ??
>>>>
>>>> Some of the appcds tests by design have some classes which fail 
>>>> verification. Without the change, the following assert would fail:
>>>>
>>>> void vtableEntry::verify(klassVtable* vt, outputStream* st) {
>>>>    Klass* vtklass = vt->klass();
>>>>    if (vtklass->is_instance_klass() &&
>>>>       (InstanceKlass::cast(vtklass)->major_version() >= 
>>>> klassVtable::VTABLE_TRANSITIVE_OVERRIDE_VERSION)) {
>>>> *assert*(method() != NULL, "must have set method");
>>>>    }
>>>
>>> Okay so you need to exclude for that case but ...
>>>
>>>> Here's the call stack during dynamic CDS dump to the above function:
>>>>
>>>> vtableEntry::verify(klassVtable *, outputStream *) : void
>>>>      klassVtable::verify(outputStream *, bool) : void
>>>>          InstanceKlass::verify_on(outputStream *) : void
>>>>              Klass::verify() : void
>>>>                  ClassLoaderData::verify() : void
>>>>                      ClassLoaderDataGraph::verify() : void
>>>>                          Universe::verify(enum VerifyOption, const 
>>>> char *) : void
>>>>                              Universe::verify(const char *) : void
>>>> DynamicArchiveBuilder::verify_universe(const char *) : void
>>>> DynamicArchiveBuilder::doit() : void (2 matches)
>>>> VM_PopulateDynamicDumpSharedSpace::doit() : void
>>>> VM_Operation::evaluate() : void
>>>>
>>>> The is_linked() function is implemented as:
>>>>
>>>>      bool is_linked() const                   { return _init_state 
>>>> >= linked; }
>>>>
>>>> which includes 'initialization_error'.
>>>
>>> ... AFAICS Universe::verify is call from a number of places, all of 
>>> which would reach this modified code. As that existing code has 
>>> included classes that are in the error state then it seems to me 
>>> that unless you can prove otherwise you need to keep the existing 
>>> code as-is for the non-CDS dump case.
>>>
>>>> A previous version of the changeset (including the change in 
>>>> question in instanceKlass.cpp) passed tier1 - 4 testing. Let me 
>>>> know if I should run other tests to make sure the change is good.
>>>
>>> There may well not be a test that covers this.
>>>
>>> Thanks,
>>> David
>>>
>>>>>
>>>>> Also can I suggest (as you are touching this code) to delete the 
>>>>> ancient comment:
>>>>>
>>>>> 3580     // $$$ This used to be done only for m/s collections. 
>>>>> Doing it
>>>>> 3581     // always seemed a valid generalization.  (DLD -- 6/00)
>>>>
>>>> I'm glad that you suggested to remove the above comment. I've no 
>>>> clue what it means either.
>>>>
>>>> updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.02/
>>>>
>>>> thanks,
>>>> Calvin
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> I also updated the test to test the shutdown hook and 
>>>>>> System.exit() scenarios.
>>>>>>
>>>>>> All appcds tests passed on my linux host. I'll run more tests 
>>>>>> using mach5.
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Calvin
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> What do you think?
>>>>>>>
>>>>>>> - Ioi
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>>
>>>>>>>>> MetaspaceShared::link_and_cleanup_shared_classes() has been 
>>>>>>>>> modified to handle both static and dynamic CDS dump. For 
>>>>>>>>> dynamic CDS dump, only classes loaded by the builtin class 
>>>>>>>>> loaders will be linked. Local performance testing using javac 
>>>>>>>>> on HelloWorld.java shows an improvement of >5%.
>>>>>>>>>
>>>>>>>>> Passed tier1 - 4 tests.
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Calvin
>>>>>>>>>
>>>>>>>
>

More information about the core-libs-dev mailing list