Looking for a sponsor to review changes made to two unit tests under jdk/test
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 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>
> <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
> .*./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