[11] RFR JDK-8198249: Remove deprecated Runtime::runFinalizersOnExit and System::runFinalizersOnExit

[mandy chung]

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