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
