Looking for a sponsor to review changes made to two unit tests under jdk/test
Mani Sarkar
sadhak001 at gmail.com
Fri Apr 5 01:27:54 UTC 2013
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.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>) and
>>
>> Edward Yue Shung Wong (edward.ys.wong at gmail.com
>> <mailto:edward.ys.wong at gmail.**com <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 <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 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.holmes at oracle.com
>> <mailto:david.holmes at oracle.**com <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>> 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!*
More information about the core-libs-dev
mailing list