Looking for a sponsor to review changes made to two unit tests under jdk/test

[Mani Sarkar]

Hi David,

Sorry for not getting back earlier. Here's the changes to
/jdk8_tl/jdk/test/java/lang/ref/Basic.java that you suggested earlier.
-------------------x---------------------

diff -r 16f63a94c231 test/java/lang/ref/Basic.java
--- a/test/java/lang/ref/Basic.java Fri Apr 05 18:20:12 2013 -0700
+++ b/test/java/lang/ref/Basic.java Sun Apr 07 00:27:55 2013 +0100
@@ -29,7 +29,7 @@

 import java.lang.ref.*;
 import java.util.Vector;
-
+import java.util.concurrent.CountDownLatch;

 public class Basic {

@@ -71,10 +71,11 @@
         });
     }

-    static void createNoise() throws InterruptedException {
+    static void createNoise(final CountDownLatch complete) throws
InterruptedException {
         fork(new Runnable() {
             public void run() {
                 keep.addElement(new PhantomReference(new Object(), q2));
+                complete.countDown();
             }
         });
     }
@@ -101,9 +102,11 @@
         for (int i = 1;; i++) {
             Reference r;

-            createNoise();
++           CountDownLatch noiseComplete = new CountDownLatch(1);
++           createNoise(noiseComplete);
+
             System.err.println("GC " + i);
-            Thread.sleep(10);
+            noiseComplete.await();
             System.gc();
             System.runFinalization();

-------------------x---------------------

Its still implemented with CountdownLatch, but as you suggest we implement
the above via Semaphore it will have to be the next version from me -
CountdownLatch was suggested by many at the TestFest last month but
personally I benefit from getting exposed to different techniques. I'll
back to you with a solution applied using Semaphore.

Cheers,
mani

On Fri, Apr 5, 2013 at 2:40 AM, David Holmes <david.holmes at oracle.com>wrote:

> On 5/04/2013 11:27 AM, Mani Sarkar wrote:
>
>> Hi David,
>>
>> I'll reply in more detail later but to respond to your comment on:
>>
>>  > I would not add the extra methods around the cdl.await() and
>> cdl.countDown() as they just obscure things
>> In general its meant to do the opposite. We come across a lot of code
>> that does not express intent, and the purpose of encapsulating such
>> blocks (even if its a single line) is to make the flow verbose and
>> readable - I have seen similar code-snippets in the Hotspot (C/C++
>> codebase) making it easy to follow what is going on. One of the things I
>> picked up from TestFest was to make the tests more legible, logical and
>> easy to maintain and scale - it was our intent when we changed this test.
>>
>
> Sorry, createNoiseHasFinishedAddingTo**Keep is certainly verbose but not
> readable. This would be much more readable:
>
> Countdownlatch noiseComplete = new ...
> createNoise(noiseComplete);
> ...
> noiseComplete.await();
>
> and:
>
> static void createNoise(final CountDownLatch complete) throws
>        InterruptedException {
>    ...
>    complete.countDown();
> }
>
> just giving the latch a meaningful name is sufficient to convey intent.
>
> Note: in this example a Semaphore is probably a better choice than a
> CountDownLatch.
>
> Cheers,
> David
>
>  I'll come back with responses for your other comments.
>>
>> Cheers
>> mani
>>
>


-- 
*Twitter:* @theNeomatrix369
*Blog:* http://neomatrix369.wordpress.com
*JUG activity:* LJC Advocate (@adoptopenjdk & @adoptajsr programs)
*Meet-a-Project:* https://github.com/MutabilityDetector
*Devoxx UK 2013 was a grand success:*
http://www.devoxx.com/display/UK13/Home
*Don't chase success, rather aim for "Excellence", and success will come
chasing after you!*