JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-01 Thread Mandy Chung
Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ This fixes the finalization implementation to invoke the finalize method via shared secret so that it will call the same method as the bytecode invocation. The current implementation uses JNI_GetMethodID to find the

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-01 Thread mark . reinhold
2013/11/1 4:15 -0700, mandy.ch...@oracle.com: > http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finalizer.java, at line 97 you look up the JavaLangAccess object every single time. Is it worth caching that earlier, maybe when the finalize threa

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-01 Thread Mandy Chung
On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finalizer.java, at line 97 you look up the JavaLangAccess object every single time. Is it wort

RE: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-02 Thread Jeroen Frijters
a.net > Subject: JDK-8027351: (ref) Base's class finalize method not invoked if > a private finalize method exists in its subclass > > Webrev at: > http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ > > This fixes the finalization implementation to invoke t

RE: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-02 Thread Jeroen Frijters
Of Mandy Chung > Sent: Friday, November 1, 2013 22:11 > To: mark.reinh...@oracle.com > Cc: core-libs-dev@openjdk.java.net > Subject: Re: JDK-8027351: (ref) Base's class finalize method not invoked > if a private finalize method exists in its subclass > > On 11/1/13

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-02 Thread Alan Bateman
On 01/11/2013 21:11, Mandy Chung wrote: I was expecting that would get optimized during runtime and it's a simple getter method. It's a good suggestion to cache it at the finalize thread start time and here is the revised webrev: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-02 Thread Peter Levart
On 11/01/2013 10:11 PM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finalizer.java, at line 97 you look up the JavaLan

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread David Holmes
Hi Mandy, On 2/11/2013 7:11 AM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finalizer.java, at line 97 you look up the

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread Mandy Chung
On 11/2/2013 3:41 AM, Peter Levart wrote: On 11/01/2013 10:11 PM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: In Finali

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread Mandy Chung
On 11/3/2013 5:32 PM, David Holmes wrote: Hi Mandy, On 2/11/2013 7:11 AM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.00/ Looks good. Just one question: I

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread David Holmes
Hi Mandy, I'll wait for the updated webrev. FYI I wasn't suggesting any VM changes regarding GetMethodID. I assumed, incorrectly, that once you had the method ID you could query to see if it was private. That said if you called GetMethodID on Object.class and then did the CallVoidMethod(env,

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread Peter Levart
On 11/04/2013 05:45 AM, Mandy Chung wrote: On 11/3/2013 5:32 PM, David Holmes wrote: Hi Mandy, On 2/11/2013 7:11 AM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/w

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-03 Thread Peter Levart
On 11/04/2013 07:52 AM, Peter Levart wrote: Also VM.awaitBooted seems inherently risky as a general method as you would have to make sure that it is never called by the main VM initialization thread. Perhaps handle this in sun.misc.SharedSecrets.getJavaLangAccess so it is less 'general'? Tha

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Peter Levart
On 11/04/2013 05:45 AM, Mandy Chung wrote: That said I think Peter may be right that there could be races with agents triggerring explicit finalization requests early in the VM initialization process - which means any blocking operation dependent on other parts of the initialization sequence co

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Alan Bateman
On 04/11/2013 08:33, Peter Levart wrote: : What about the following helper class in java.lang package: If public then it leaks into the Java SE API. I think using SharedSecrets should be okay, assuming we can sort out any potential races getting to JavaLangAccess. -Alan.

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Peter Levart
On 11/04/2013 09:48 AM, Alan Bateman wrote: On 04/11/2013 08:33, Peter Levart wrote: : What about the following helper class in java.lang package: If public then it leaks into the Java SE API. I think using SharedSecrets should be okay, assuming we can sort out any potential races getting to

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Paul Sandoz
On Nov 2, 2013, at 10:03 AM, Alan Bateman wrote: > On 01/11/2013 21:11, Mandy Chung wrote: >> >> I was expecting that would get optimized during runtime and it's a simple >> getter method. It's a good suggestion to cache it at the finalize thread >> start time and here is the revised webrev:

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Mandy Chung
On 11/3/13 10:52 PM, Peter Levart wrote: On 11/04/2013 05:45 AM, Mandy Chung wrote: On 11/3/2013 5:32 PM, David Holmes wrote: Hi Mandy, On 2/11/2013 7:11 AM, Mandy Chung wrote: On 11/1/13 1:37 PM, mark.reinh...@oracle.com wrote: 2013/11/1 4:15 -0700, mandy.ch...@oracle.com: http://cr.openj

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Mandy Chung
Thank you all for the suggestions. Before the VM initialization is completed, any agent ought to be careful in what things it can do and what shouldn't do. I think it's reasonable to ignore System.runFinalization if it's called at early startup phase. I'm unclear if there is any use case tha

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread David Holmes
Hi Mandy, This all seems quite reasonable to me. Two minor nits: 1. The "delay ntil" typo in Finalizer.java is still present. 2. In VM.java. booted need not be volatile now that it is only accessed within a locked region. Also awaitBooted might as well be void as it can only ever return true

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Mandy Chung
On 11/4/2013 6:03 PM, David Holmes wrote: Hi Mandy, This all seems quite reasonable to me. Two minor nits: 1. The "delay ntil" typo in Finalizer.java is still present. Missed that :( 2. In VM.java. booted need not be volatile now that it is only accessed within a locked region. Also await

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread David Holmes
On 5/11/2013 12:21 PM, Mandy Chung wrote: On 11/4/2013 6:03 PM, David Holmes wrote: Hi Mandy, This all seems quite reasonable to me. Two minor nits: 1. The "delay ntil" typo in Finalizer.java is still present. Missed that :( 2. In VM.java. booted need not be volatile now that it is only

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Mandy Chung
On 11/4/2013 6:29 PM, David Holmes wrote: 2. In VM.java. booted need not be volatile now that it is only accessed within a locked region. Oops my bad! The accessor wasn't synchronized but now is. Your call whether to leave as is or revert to previous. I'm fine with making isBooted() to a syn

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-04 Thread Peter Levart
On 11/05/2013 03:21 AM, Mandy Chung wrote: On 11/4/2013 6:03 PM, David Holmes wrote: Hi Mandy, This all seems quite reasonable to me. Two minor nits: 1. The "delay ntil" typo in Finalizer.java is still present. Missed that :( 2. In VM.java. booted need not be volatile now that it is only

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Paul Sandoz
On Nov 5, 2013, at 3:21 AM, Mandy Chung wrote: >> 2. In VM.java. booted need not be volatile now that it is only accessed >> within a locked region. Also awaitBooted might as well be void as it can >> only ever return true. >> > > Fixed. Revised webrev at: > http://cr.openjdk.java.net/~mchung

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Alan Bateman
On 05/11/2013 02:21, Mandy Chung wrote: Fixed. Revised webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.03/ I looked at the latest webrev. Having runFinalization and runAllFinalizers be a no-open during initialization is reasonable (it should never happen). I agre

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Paul Sandoz
On Nov 5, 2013, at 8:26 AM, Peter Levart wrote: > > P.S. I'm curious about the strange seemingly unneeded code fragments in > FinalizerThread and both Runnables. For example: > > 169 forkSecondaryFinalizer(new Runnable() { > 170*private volatile boolean running;* > 171 publi

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Chris Hegarty
On 05/11/2013 10:59, Paul Sandoz wrote: On Nov 5, 2013, at 8:26 AM, Peter Levart wrote: P.S. I'm curious about the strange seemingly unneeded code fragments in FinalizerThread and both Runnables. For example: 169 forkSecondaryFinalizer(new Runnable() { 170*private volatile boolean ru

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Peter Levart
On 11/05/2013 12:33 PM, Chris Hegarty wrote: On 05/11/2013 10:59, Paul Sandoz wrote: On Nov 5, 2013, at 8:26 AM, Peter Levart wrote: P.S. I'm curious about the strange seemingly unneeded code fragments in FinalizerThread and both Runnables. For example: 169 forkSecondaryFinalizer(n

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Mandy Chung
On 11/5/2013 1:11 AM, Paul Sandoz wrote: On Nov 5, 2013, at 3:21 AM, Mandy Chung wrote: 2. In VM.java. booted need not be volatile now that it is only accessed within a locked region. Also awaitBooted might as well be void as it can only ever return true. Fixed. Revised webrev at: http://

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Mandy Chung
On 11/5/2013 2:38 AM, Alan Bateman wrote: On 05/11/2013 02:21, Mandy Chung wrote: Fixed. Revised webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8027351/webrev.03/ I looked at the latest webrev. Having runFinalization and runAllFinalizers be a no-open during initialization is re

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread David Holmes
Ship it! :) And again apologies for sending you down the wrong path on the volatile. David On 6/11/2013 6:25 AM, Mandy Chung wrote: On 11/5/2013 2:38 AM, Alan Bateman wrote: On 05/11/2013 02:21, Mandy Chung wrote: Fixed. Revised webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/80

Re: JDK-8027351: (ref) Base's class finalize method not invoked if a private finalize method exists in its subclass

2013-11-05 Thread Mandy Chung
On 11/5/2013 4:23 PM, David Holmes wrote: Ship it! :) Thanks for the review. And again apologies for sending you down the wrong path on the volatile. No apology needed. I missed its usage in sun.misc.VM. Mandy David On 6/11/2013 6:25 AM, Mandy Chung wrote: On 11/5/2013 2:38 AM, Ala