RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

Claes Redestad
Hi,

the
java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java
has
started timing out locallty on my machine, and analyzing why it seems it
simply has
added enough test cases recently to hit the default 120s timeout.

Quickly analyzing what is taking so much time I ran into an inefficiency
in TestNGs
assertEquals(Iterator<?>, Iterator<?>) implementation, where at least
one error message
string is created unconditionally in an inner loop, leading to quite a
bit of allocation
pressure in a test like WhileOpTest.

On my machine, avoiding this extra work speeds up the test from taking
around ~130s
to ~100s. A good improvement, but still somewhat close to the default
120s timeout.

So as a fix I propose:

- work around this by providing simplified assert methods in the
LambdaTestHelper that
avoid the excessively allocating methods in TestNG
- increasing the timeout of WhileOpTest to 240s

http://cr.openjdk.java.net/~redestad/8134459/open.00/

Regards

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

roger riggs
Hi Claes,

Looks ok, a couple of nits.

LambdaTestHelpers:
line 57: Please break the long string to make future side-by-side
reviews easier.

line 322:  Perhaps "Iterator contents differ" instead of "iterators
differ" so its clear
the contents of the iterators are different, not the instances.

Having the indices might be useful for debugging, but they aren't ever
supposed to be different.

Regards, Roger


On 1/9/2018 4:15 PM, Claes Redestad wrote:

> Hi,
>
> the
> java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java
> has
> started timing out locallty on my machine, and analyzing why it seems
> it simply has
> added enough test cases recently to hit the default 120s timeout.
>
> Quickly analyzing what is taking so much time I ran into an
> inefficiency in TestNGs
> assertEquals(Iterator<?>, Iterator<?>) implementation, where at least
> one error message
> string is created unconditionally in an inner loop, leading to quite a
> bit of allocation
> pressure in a test like WhileOpTest.
>
> On my machine, avoiding this extra work speeds up the test from taking
> around ~130s
> to ~100s. A good improvement, but still somewhat close to the default
> 120s timeout.
>
> So as a fix I propose:
>
> - work around this by providing simplified assert methods in the
> LambdaTestHelper that
> avoid the excessively allocating methods in TestNG
> - increasing the timeout of WhileOpTest to 240s
>
> http://cr.openjdk.java.net/~redestad/8134459/open.00/
>
> Regards
>
> /Claes

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

Paul Sandoz
In reply to this post by Claes Redestad
This might also improve other tests using the same assert.

LambdaTestHelpers
--

s/if(/if (

+        int i = -1;
+        while (actual.hasNext() && expected.hasNext()) {
+            i++;
+            Object e = expected.next();
+            Object a = actual.next();
+            assertEquals(a, e, "Iterators differ");
+
+        }


‘i' is redundant. I presume might have wanted to use it to print out the index on element assertion failure, but it would of course induce garbage.

Paul.


> On 9 Jan 2018, at 13:15, Claes Redestad <[hidden email]> wrote:
>
> Hi,
>
> the java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java has
> started timing out locallty on my machine, and analyzing why it seems it simply has
> added enough test cases recently to hit the default 120s timeout.
>
> Quickly analyzing what is taking so much time I ran into an inefficiency in TestNGs
> assertEquals(Iterator<?>, Iterator<?>) implementation, where at least one error message
> string is created unconditionally in an inner loop, leading to quite a bit of allocation
> pressure in a test like WhileOpTest.
>
> On my machine, avoiding this extra work speeds up the test from taking around ~130s
> to ~100s. A good improvement, but still somewhat close to the default 120s timeout.
>
> So as a fix I propose:
>
> - work around this by providing simplified assert methods in the LambdaTestHelper that
> avoid the excessively allocating methods in TestNG
> - increasing the timeout of WhileOpTest to 240s
>
> http://cr.openjdk.java.net/~redestad/8134459/open.00/
>
> Regards
>
> /Claes

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

Claes Redestad
Hi Paul,

On 2018-01-09 23:23, Paul Sandoz wrote:
> This might also improve other tests using the same assert.

Not unlikely.

>
> LambdaTestHelpers
> --
>
> s/if(/if (
>
> +        int i = -1;
> +        while (actual.hasNext() && expected.hasNext()) {
> +            i++;
> +            Object e = expected.next();
> +            Object a = actual.next();
> +            assertEquals(a, e, "Iterators differ");
> +
> +        }
>
>
> ‘i' is redundant. I presume might have wanted to use it to print out the index on element assertion failure, but it would of course induce garbage.

Fixed in place

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8134459: java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java timed out

joe darcy
In reply to this post by Claes Redestad
Hello,

Making tests run faster is all good of course. However, I'm leery of
bumping up the timeouts; I'd prefer the tests be split into multiple
parts if there is a chance it will take many minutes to run. (Having
individual regression tests run in at most a minute or two helps avoid
Amdahl's law problems when running the regression tests with concurrency.)

Thanks,

-Joe

On 1/9/2018 1:15 PM, Claes Redestad wrote:

> Hi,
>
> the
> java/util/stream/test/org/openjdk/tests/java/util/stream/WhileOpTest.java
> has
> started timing out locallty on my machine, and analyzing why it seems
> it simply has
> added enough test cases recently to hit the default 120s timeout.
>
> Quickly analyzing what is taking so much time I ran into an
> inefficiency in TestNGs
> assertEquals(Iterator<?>, Iterator<?>) implementation, where at least
> one error message
> string is created unconditionally in an inner loop, leading to quite a
> bit of allocation
> pressure in a test like WhileOpTest.
>
> On my machine, avoiding this extra work speeds up the test from taking
> around ~130s
> to ~100s. A good improvement, but still somewhat close to the default
> 120s timeout.
>
> So as a fix I propose:
>
> - work around this by providing simplified assert methods in the
> LambdaTestHelper that
> avoid the excessively allocating methods in TestNG
> - increasing the timeout of WhileOpTest to 240s
>
> http://cr.openjdk.java.net/~redestad/8134459/open.00/
>
> Regards
>
> /Claes