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

[Ioi Lam]

On 2/27/20 10:14 PM, Calvin Cheung wrote:
> Hi Ioi,
>
> On 2/27/20 8:22 PM, Ioi Lam wrote:
>> Hi Calvin,
>>
>> This looks good to me.
> Thanks for taking another look.
>>
>> I would suggest adding the something like this to the test case:
>>
>> class Child extends Parent {
>>     int get() {return 2;}
>>     int dummy() {
>>         // When Child is linked during dynamic dump (when the VM is 
>> shutting
>>         // down), it will be verified, which will recursively cause 
>> Child2/Parent2
>>         // to be loaded and verified.
>>         // We want to make sure that this is done at a point in the 
>> VM lifecycle where
>>         // it's still safe to do so.
>>         Parent2 x = new Child2();
>>         return x.get();
>>     }
>> }
>
> The following updated webrev includes the above changes in the test case:
>
>     http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.02/
>
Looks good. Thanks!

- Ioi

> thanks,
>
> Calvin
>
>>
>> Thanks
>> - Ioi
>>
>> On 2/27/20 4:48 PM, 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/
>>>
>>> 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
>>>>>>
>>>>
>>