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