Looking for a sponsor to review changes made to two unit tests under jdk/test (original) (raw)

David Holmes david.holmes at oracle.com
Fri Apr 5 01:40:46 UTC 2013

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, createNoiseHasFinishedAddingToKeep 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 On Fri, Apr 5, 2013 at 2:07 AM, David Holmes <david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com>> wrote: Hi Mani, Patches came through ok. I would not add the extra methods around the cdl.await() and cdl.countDown() as they just obscure things in my opinion. But that aside the latch is not needed. The fork() method starts a thread and joins it. So when createNoise() returns we already know for certain that the "noise has been created". What the sleep is doing is giving the GC a chance to run. David

On 5/04/2013 10:55 AM, Mani Sarkar wrote: Thanks David, Here are the patches, let me know if they have come in fine: _1) test/java/lang/ref/Basic._java.patch - changed to not use Thread.sleep() any more rather use the _java.util.concurrent._CountdownLatch functionality Authors: Mani (sadhak001 at gmail.com <mailto:sadhak001 at gmail.com> <mailto:sadhak001 at gmail.com <mailto:sadhak001 at gmail.com>>) and Edward Yue Shung Wong (edward.ys.wong at gmail.com <mailto:edward.ys.wong at gmail.com> <mailto:edward.ys.wong at gmail._com_ _<mailto:edward.ys.wong at gmail.com>>) ------------x--------------- diff -r 38e1821c4472 test/java/lang/ref/Basic.java _--- a/test/java/lang/ref/Basic.javaWed Mar 06 18:35:51 2013 +0100 _+++ b/test/java/lang/ref/Basic.javaSat Mar 23 14:51:25 2013 +0000 @@ -29,7 +29,7 @@ import java.lang.ref.*; import java.util.Vector; - _+import java.util.concurrent.CountDownLatch; public class Basic { @@ -64,22 +64,22 @@ fork(new Runnable() { public void run() { _System.err.println("References: W " + rw.get() - + ", W2 " + rw2.get() - + ", P " + rp.get() - + ", P2 " + rp2.get()); + + ", W2 " + rw2.get() + + ", P " + rp.get() + + ", P2 " + rp2.get()); } }); } - static void createNoise() throws InterruptedException { + static void createNoise(final CountDownLatch cdl) throws InterruptedException { fork(new Runnable() { public void run() { keep.addElement(new PhantomReference(new Object(), q2)); + createNoiseHasFinishedAddingTo_Keep(cdl); } }); } - public static void main(String[] args) throws Exception { fork(new Runnable() { @@ -97,13 +97,16 @@ int ndq = 0; boolean prevFinalized = false; - outer: + outer: for (int i = 1;; i++) { Reference r; - createNoise(); + CountDownLatch inQueueWaitLatch = new CountDownLatch(1); + createNoise(inQueueWaitLatch); + System.err.println("GC " + i); - Thread.sleep(10); + waitUntilCreateNoiseHasFinishe_d(inQueueWaitLatch); + System.gc(); System.runFinalization(); @@ -137,7 +140,7 @@ if (ndq != 3) { throw new Exception("Expected to dequeue 3 reference objects," - + " but only got " + ndq); + + " but only got " + ndq); } if (! Basic.finalized) { @@ -146,4 +149,13 @@ } + + private static void createNoiseHasFinishedAddingTo_Keep(CountDownLatch cdl) { + cdl.countDown(); + } + + private static void waitUntilCreateNoiseHasFinishe_d(CountDownLatch cdl) throws InterruptedException { + cdl.await(); + } + } ------------x---------------- _2) test/java/lang/Runtime/exec/_LotsOfOutput.java.patch - refactor-ing and tidy-ing of existing code (removing string literals and replacing with constants, etc...) Author: Edward Yue Shung Wong (edward.ys.wong at gmail.com <mailto:edward.ys.wong at gmail.com> <mailto:edward.ys.wong at gmail._com_ _<mailto:edward.ys.wong at gmail.com>>) ------------x---------------- _diff -r 38e1821c4472 test/java/lang/Runtime/exec/LotsOfOutput.java _--- a/test/java/lang/Runtime/exec/LotsOfOutput.javaWed Mar 06 18:35:51 2013 +0100 _+++ b/test/java/lang/Runtime/exec/LotsOfOutput.javaSat Mar 23 15:48:46 2013 +0000 @@ -33,17 +33,24 @@ public class LotsOfOutput { static final String CAT = "/usr/bin/cat"; - public static void main(String[] args) throws Exception{ - if (File.separatorChar == '\' || // Windows - !new File(CAT).exists()) // no cat + static final int MEMORYGROWTHLIMIT = 1000000; + + public static void main(String[] args) throws Exception { + if (isWindowsOrCatNotAvailable()) { return; + } + Process p = Runtime.getRuntime().exec(CAT + " /dev/zero"); _long initMemory = Runtime.getRuntime().totalMemory(); - for (int i=1; i< 10; i++) {_ _+ for (int i = 1; i < 10; i++) {_ _Thread.sleep(100);_ _- if (Runtime.getRuntime()._totalMemory() > initMemory + 1000000) - throw new Exception("Process consumes memory."); _+ if (Runtime.getRuntime().totalMemory() > initMemory + MEMORYGROWTHLIMIT) + throw new Exception("Runtime memory has grown more than: " + MEMORYGROWTHLIMIT); } } + + private static boolean isWindowsOrCatNotAvailable() { + return File.separatorChar == '\' || !new File(CAT).exists(); + } } ------------x---------------- Any queries please let me know. Thanks. Regards, mani On Fri, Apr 5, 2013 at 1:38 AM, David Holmes <david.holmes at oracle.com <mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle._com_ _<mailto:david.holmes at oracle.com>>> wrote: Hi Mani, Attachments get stripped so you will need to inline the patches in your email to the list. Thanks, David On 5/04/2013 9:07 AM, Mani Sarkar wrote: Hi all, I'd like to rectify that I;m a contributor (and not a committer as mentioned earlier), so I don't have access to the webrev system but would love to have these patches hosted on them when a sponsor becomes available. Cheers, mani On Thu, Apr 4, 2013 at 11:57 PM, Mani Sarkar <sadhak001 at gmail.com <mailto:sadhak001 at gmail.com> <mailto:sadhak001 at gmail.com <mailto:sadhak001 at gmail.com>>> wrote: Hi all, During #TestFest in London last month we hacked away on jtreg and tests in the OpenJDK. Myself and another participant managed to change two unit tests. I haven't looked for a sponsor in the past so I'm fairly new to the process, hence my email on the list is to request for someone to review, and process these patches further. I'm a committer (signed OCA). Here's details of the changes made to the tests under the ../jdk8tl/jdkfolders: _1) test/java/lang/ref/Basic.__java.patch -_ changed to not use Thread.sleep() any more rather use the _java.util.concurrent._CountdownLatch functionality 2) _test/java/lang/Runtime/exec/__LotsOfOutput.java.patch -_ refactor-ing and tidy-ing of existing code (removing string literals and replacing with constants, etc...) Please let me know what you would like me to do next with these patches. Regards, mani -- Twitter: @theNeomatrix369 Blog: http://neomatrix369.wordpress.__com <http://neomatrix369._wordpress.com_ _<http://neomatrix369.wordpress.com>> JUG activity: LJC Advocate (@adoptopenjdk & @adoptajsr programs) Meet-a-Project: https://github.com/__MutabilityDetector <https://github.com/_MutabilityDetector> <https://github.com/_MutabilityDetector_ _<https://github.com/MutabilityDetector>> Devoxx UK 2013 was a grand success: http://www.devoxx.com/display/__UK13/Home <http://www.devoxx.com/display/_UK13/Home> <http://www.devoxx.com/_display/UK13/Home_ _<http://www.devoxx.com/display/UK13/Home>> *Don't chase success, rather aim for "Excellence", and success will come chasing after you!*

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

More information about the core-libs-dev mailing list