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. I'll come back with responses for your other comments. Cheers mani On Fri, Apr 5, 2013 at 2:07 AM, David Holmes <david.hol...@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 (sadhak...@gmail.com <mailto:sadhak...@gmail.com>) and >> >> Edward Yue Shung Wong (edward.ys.w...@gmail.com >> <mailto:edward.ys.wong@gmail.**com <edward.ys.w...@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.w...@gmail.com >> <mailto:edward.ys.wong@gmail.**com <edward.ys.w...@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 MEMORY_GROWTH_LIMIT = 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 + >> MEMORY_GROWTH_LIMIT) >> + throw new Exception("Runtime memory has grown more >> than: " + MEMORY_GROWTH_LIMIT); >> } >> } >> + >> + 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.hol...@oracle.com >> <mailto:david.holmes@oracle.**com <david.hol...@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 >> <sadhak...@gmail.com <mailto:sadhak...@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 >> .*./jdk8_tl/jdk*folders: >> >> 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!*
