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 ?? 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/cla...

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

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla...

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"? --- 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 ?? 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) 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

Hi Ioi, On 28/02/2020 2:09 pm, Ioi Lam wrote:

Hi David,

There classes are usually loaded during verification. They are not linked because they are referenced only by methods that were never executed during dynamic dumping. You can see an example here

http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/test/hotspot/jtr...

The "Child" and "Parent" classes are loaded during verification but never linked.

Note that the LinkClassApp has been verified during dump time, so we can skip most of the bytecode verification steps during run time. However, when LinkClassApp is linked at run time, Child/Parent will be resolved to ensure that Child is still a subclass of Parent (otherwise the bytecodes in LinkClassApp.test() will be invalid). This is done here:

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/class...

Before this fix, Child/Parent will be loaded from classfiles, which slows down the link time of LinkClassApp.

So in essence we do extra work at dynamic dump time, and increase the size of the archive, so that we can speedup the subsequent link time of LinkClassApp.

(I am adding this info to the bug report for clarification).

Thanks for doing that. David

Thanks - Ioi

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

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

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

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla...

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

---

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

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)

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

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

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

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla...

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

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

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

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla...

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

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/cla... > > >             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/runti... > > http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla... > > > 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 >>> >

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/cla... >>> >>> >>>             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/runti... >>> >>> http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla... >>> >>> >>> 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 >>>>> >>>

Hi Ioi, On 2/27/20 8:22 PM, Ioi Lam wrote:

Hi Calvin,

This looks good to me. Thanks for taking another look.

I would suggest adding the something like this to the test case:

class Child extends Parent {     int get() {return 2;}     int dummy() {         // When Child is linked during dynamic dump (when the VM is shutting         // down), it will be verified, which will recursively cause Child2/Parent2         // to be loaded and verified.         // We want to make sure that this is done at a point in the VM lifecycle where         // it's still safe to do so.         Parent2 x = new Child2();         return x.get();     } }

The following updated webrev includes the above changes in the test case:     http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.02/ thanks, Calvin

Thanks - Ioi

On 2/27/20 4:48 PM, 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/cla...

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

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/cla...

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/

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