RFR(S): 8232081: Try to link all classes during dynamic CDS dump
Calvin Cheung
calvin.cheung at oracle.com
Fri Mar 6 16:44:21 UTC 2020
Hi Ioi,
On 3/5/20 9:16 PM, Ioi Lam wrote:
> Hi Calvin,
>
> Looks good.
Thanks for taking another look.
>
> In JDK-8240244, I removed "ik->loader_type() != 0" because it's
> unclear what it means. I replaced with a new method
> InstanceKlass::is_shared_unregistered_class(). I think you can use
> this in your patch instead of adding loader_type() back.
I'll make the change and do more testing before push.
thanks,
Calvin
>
> No need for new webrev.
>
> Thanks
> - Ioi
>
>
> On 3/5/20 11:48 AM, Calvin Cheung wrote:
>>
>> 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