On 2/15/18 6:11 PM, David Holmes wrote:
Hi Mandy,
Good to see this go. A few minor comments.
First I've added comments on the CSR as some of the doc changes don't
read correctly.
Thanks for catching it. What do you think about this revision:
* <p> All registered {@link #addShutdownHook shutdown hooks}, if any,
* are started in some unspecified order and allowed to run concurrently
* until they finish. Once this is done the virtual machine
* {@link #halt halts}.
*
* <p> If this method is invoked after the virtual machine has started
* shutdown hooks then if shutdown hooks have already been run and
* the status is nonzero then this method halts the virtual machine
* with the given status code; otherwise, this method blocks indefinitely
* (i.e. if shutdown hooks are being run or if shutdown hooks have already
* been run and the status is zero).
I'll update the CSR once we agree on one version.
src/hotspot/share/runtime/thread.cpp
This comment doesn't read correctly:
! // won't be run. Note that if a shutdown hook was registered
// was called, the Shutdown class would have already been loaded
! // (Runtime.addShutdownHook will load it).
delete "was called, "
Deleted.
---
src/java.base/share/classes/java/lang/Shutdown.java
This was a bit confusing. :) I wasn't at all sure you needed the
COMPLETED state (which is really a HOOKS_HAVE_BEEN_STARTED state). But
I see it allows for a second exit(<non-zero) call to be given
preference to the initial exit() call (whether non-zero or not). That
seems to maintain existing behaviour.
Yes this maintains the existing behavior.
---
src/java.base/share/classes/java/lang/ref/Finalizer.java
private void remove() {
synchronized (lock) {
if (unfinalized == this) {
- if (this.next != null) {
unfinalized = this.next;
- } else {
- unfinalized = this.prev;
- }
This seems unrelated to this change. Is this the optimization Martin
proposed?
The only case when this.next == null and this.prev != null could happen
when runAllFinalizers were involved. This case is no longer needed.
! This method is used by both runFinalization.
The former method invokes all pending finalizers, while the
latter
invokes all uninvoked finalizers if on-exit finalization has
been
enabled.
As Stuart said remove "both" in the modified line. But the following
line also needs changing or deleting. As does the one after that:
116 These two methods could have been implemented by
offloading their work
as there are no longer two methods.
Good catch. Updated.
Otherwise this all seems okay. I was surprised none of this really
impacted the VM. :)
This is all done in the library side. The VM did have the
run_finalization_at_exit function but never used.
thanks
Mandy
