RFR(S): 8232081: Try to link all classes during dynamic CDS dump
David Holmes
david.holmes at oracle.com
Thu Mar 5 07:34:40 UTC 2020
On 5/03/2020 3: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/
No further comments from me. Thanks for cleaning that up.
David
> 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