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

[David Holmes]

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
>