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

[Calvin Cheung]

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

>
> 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.
>
> ---
>
> 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");
   }

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

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.

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