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

[Calvin Cheung]

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
>>>>>>>
>>>>>