RFR(S): 8232081: Try to link all classes during dynamic CDS dump
Ioi Lam
ioi.lam at oracle.com
Fri Mar 6 05:16:33 UTC 2020
Hi Calvin,
Looks good.
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.
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