Hi Mandy,
On 16/02/2018 1:20 PM, mandy chung wrote:
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}.
This sounds good.
* <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).
This is still confusing to me (and I don't claim the original is not
also confusing!). Do we actually care/distinguish between "hooks have
been started" and "hooks have already been run" - which implies all
user-defined hooks have finished? I think the only thing that is
significant is whether hooks have been started, and then what the status
code is. So I'd suggest simply:
* <p> If this method is invoked after the virtual machine has started
* shutdown hooks and the status is nonzero then this method halts the
* virtual machine with the given status code. Otherwise, this method
* blocks indefinitely.
?
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.
The more I look at this the more I think COMPLETED is not the right term
here.
---
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.
Okay - this is far from obvious though.
! 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.
102 * It could have been implemented by offloading their work to the
s/their/the/
105 * invokers of these methods from a stalled or deadlocked
finalizer thread.
s/these methods/that method/
All other updates seem okay. I did have one further thought - in Runtime
does this change:
public void runFinalization() {
! SharedSecrets.getJavaLangRefAccess().runFinalization();
}
affect the classloading/initialization order at all?
Thanks,
David
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
