Thanks for your reply Roger. I’m fine with the changes are they are. -Chris.
On 22 Dec 2015, at 18:10, Roger Riggs <[email protected]> wrote: > Hi Chris, > > Thanks for the review > > On 12/22/2015 12:01 PM, Chris Hegarty wrote: >> Hi Roger, >> >> On 22 Dec 2015, at 16:35, Roger Riggs >> <[email protected]> >> wrote: >> >> >>> Please review improvements to the CleanerTest to improve the reliability of >>> the test. >>> >>> Webrev: >>> >>> http://cr.openjdk.java.net/~rriggs/webrev-cleanertest-8146012/ >> >> The use of WhiteBox should make the test more reliable ( rather than relying >> on generating garbage ). >> >> Is @library /lib/testlibrary necessary? I don’t see any usage of types from >> it. >> > yes, the ClassFileInstaller that enables the WhiteBox is there. >> >> Is '-Xbootclasspath/a:.’ necessary? Is this for WhiteBox, or the test >> itself? I >> don’t see why it is necessary. >> > yes, That's how the share library for the whitebox native code is found. >> >> Moving from a varargs of Semaphore to just a single Semaphore does simplify >> the code, since it is always called with a single Semaphore. >> >> The new checkCleaned does not enforce availablePermits == 1, just that a >> permit >> is available. Should it assert 1 ? >> > The tryAdvance method consumes the expected permit and the method does not > wait around to > see if it will get incremented again. So checking again would be hit or > miss. The check completes > when the semaphore is triggered. > The running time of the test would increase and I can't see it paying off > since the Cleaner > implementation is written to call it only once, so it would require some > unexpected case to trigger that bug. > > Roger > >> >> -Chris. >> >> >>> Issue: >>> >>> https://bugs.openjdk.java.net/browse/JDK-8146012 >>> >>> >>> Thanks, Roger >>> >
