RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

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

RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
Hi,

This test times out under our automated testing configurations that
include -Xcomp.

Please review my change to increase the timeout for this test.  It is
sufficient for the test configurations in question to pass on two
different local machines (Mac & Linux).

diff -r 7c04ab31b4d6
test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
--- a/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
Wed Apr 26 09:37:23 2017 -0700
+++ b/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
Thu Apr 27 12:03:33 2017 -0700
@@ -29,7 +29,7 @@
   * @library /lib/testlibrary
   * @modules java.base/jdk.internal.module
   * @build JarUtils CompilerUtils
- * @run main/timeout=240 ClassLoaderTest
+ * @run main/timeout=1200 ClassLoaderTest
   */

Thanks,
-Brent
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
Hi, Joe

The typical run time of this test is ~18s (on my modestly-equipped
laptop).  So the test does run within a reasonable amount of time, IMO -
under normal circumstances, anyway.

The increased timeout is to cover the less seldom test run
configurations involving Xcomp.  Judging by when the test was added and
the dates of the failed runs, I think this test has timed out since it
was introduced, and there has not been an Xcomp regression.

The test checks for a recursive initialization issue (8168075), and so
needs to launch JVMs using various combinations of system
classloader/security manager+policy/various module types.  For this
reason I think Xcomp hits this test particularly hard, recompiling the
startup code on every VM launch.  I think we want to maintain thorough
test coverage here, though.

Looking more into the failures, the worst case looks to be getting not
quite halfway through the test.  We should be able to get away with a
more modest increase to the timeout (600, instead of 1200), and still
have the test pass, if you would prefer.

Thanks,
-Brent

On 4/27/17 2:25 PM, joe darcy wrote:

> I understand the interest in having test pass reliably, but I don't
> think giving the test very large timeouts is the preferred way of
> accomplishing that.
>
> For all configurations, the test can now run for up to 20 minutes, up
> from 4 minutes. We want to run the entire test suite, thousands of
> tests, in about 20 minutes. The the timeout factor used for Xcomp run,
> the test would probably now be able to run for over an hour before
> timing out.
>
> I suggest making the test run faster, or seeing if there has been a
> regressions in Xcomp to make test perform more poorly there.
>
> Thanks,
>
> -Joe
>
>
> On 4/27/2017 12:08 PM, Brent Christian wrote:
>> Hi,
>>
>> This test times out under our automated testing configurations that
>> include -Xcomp.
>>
>> Please review my change to increase the timeout for this test.  It is
>> sufficient for the test configurations in question to pass on two
>> different local machines (Mac & Linux).
>>
>> diff -r 7c04ab31b4d6
>> test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
>> --- a/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
>> Wed Apr 26 09:37:23 2017 -0700
>> +++ b/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
>> Thu Apr 27 12:03:33 2017 -0700
>> @@ -29,7 +29,7 @@
>>   * @library /lib/testlibrary
>>   * @modules java.base/jdk.internal.module
>>   * @build JarUtils CompilerUtils
>> - * @run main/timeout=240 ClassLoaderTest
>> + * @run main/timeout=1200 ClassLoaderTest
>>   */
>>
>> Thanks,
>> -Brent
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
On 5/1/17 12:41 PM, Mandy Chung wrote:
> Looks like this test execs a new VM for 66 times to exhaust the
> combination of with and without security manager, with a valid or
> malformed policy, client & custom loader in unnamed, named, automatic
> module.

Right.

> I think we could take out the automatic module case as named
> module would verify it.

OK, I can do that.  I'm pretty sure an increase to the timeout will
still be needed, though.

> One refactor you can consider  by separating
> them in several @run actions.  The timeout will apply to each action
> but that does not help shorten the test execution time.

I had the same thought, and concluded that without a reduction in
overall execution time, the benefit of such a refactor is pretty limited.

Thanks,
-Brent
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

roger riggs
Hi,

Also, should it not be the case that running with -Xcomp should increase
the timeout factor
to avoid such issues?  I'm not sure how that can be assured though.

Roger


On 5/1/2017 3:47 PM, Brent Christian wrote:

> On 5/1/17 12:41 PM, Mandy Chung wrote:
>> Looks like this test execs a new VM for 66 times to exhaust the
>> combination of with and without security manager, with a valid or
>> malformed policy, client & custom loader in unnamed, named, automatic
>> module.
>
> Right.
>
>> I think we could take out the automatic module case as named
>> module would verify it.
>
> OK, I can do that.  I'm pretty sure an increase to the timeout will
> still be needed, though.
>
>> One refactor you can consider  by separating
>> them in several @run actions.  The timeout will apply to each action
>> but that does not help shorten the test execution time.
>
> I had the same thought, and concluded that without a reduction in
> overall execution time, the benefit of such a refactor is pretty limited.
>
> Thanks,
> -Brent

Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
The test runs in question are using an increased timeout factor, but
this test still needs more time than its current timeout provides.

Thanks,
-Brent

On 5/1/17 1:22 PM, Roger Riggs wrote:

> Hi,
>
> Also, should it not be the case that running with -Xcomp should increase
> the timeout factor
> to avoid such issues?  I'm not sure how that can be assured though.
>
> Roger
>
>
> On 5/1/2017 3:47 PM, Brent Christian wrote:
>> On 5/1/17 12:41 PM, Mandy Chung wrote:
>>> Looks like this test execs a new VM for 66 times to exhaust the
>>> combination of with and without security manager, with a valid or
>>> malformed policy, client & custom loader in unnamed, named, automatic
>>> module.
>>
>> Right.
>>
>>> I think we could take out the automatic module case as named
>>> module would verify it.
>>
>> OK, I can do that.  I'm pretty sure an increase to the timeout will
>> still be needed, though.
>>
>>> One refactor you can consider  by separating
>>> them in several @run actions.  The timeout will apply to each action
>>> but that does not help shorten the test execution time.
>>
>> I had the same thought, and concluded that without a reduction in
>> overall execution time, the benefit of such a refactor is pretty limited.
>>
>> Thanks,
>> -Brent
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
In reply to this post by Brent Christian-2
On 5/1/17 12:50 PM, Mandy Chung wrote:
>> On May 1, 2017, at 12:47 PM, Brent Christian <[hidden email]> wrote:
 >>>
>>> One refactor you can consider  by separating
>>> them in several @run actions.  The timeout will apply to each action
>>> but that does not help shorten the test execution time.
>>
>> I had the same thought, and concluded that without a reduction in overall execution time, the benefit of such a refactor is pretty limited.
>
> I think it would make the test easier to read and understand the cases it cover.
>

True.  I'll see what I can do, then.

-Brent

Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Jonathan Gibbons
In reply to this post by Brent Christian-2
A timeout of 1200 seconds (20 minutes) is pretty extreme.

If you are testing configurations with -Xcomp. you might do better to
use the jtreg command-line option -timeoutfactor instead.

-- Jon

On 04/27/2017 12:08 PM, Brent Christian wrote:

> Hi,
>
> This test times out under our automated testing configurations that
> include -Xcomp.
>
> Please review my change to increase the timeout for this test.  It is
> sufficient for the test configurations in question to pass on two
> different local machines (Mac & Linux).
>
> diff -r 7c04ab31b4d6
> test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
> --- a/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
> Wed Apr 26 09:37:23 2017 -0700
> +++ b/test/java/lang/ClassLoader/securityManager/ClassLoaderTest.java
> Thu Apr 27 12:03:33 2017 -0700
> @@ -29,7 +29,7 @@
>   * @library /lib/testlibrary
>   * @modules java.base/jdk.internal.module
>   * @build JarUtils CompilerUtils
> - * @run main/timeout=240 ClassLoaderTest
> + * @run main/timeout=1200 ClassLoaderTest
>   */
>
> Thanks,
> -Brent

Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Jonathan Gibbons
In reply to this post by Brent Christian-2
So how long does the test typically take to run, with and without -Xcomp.

If you are running with -Xcomp /and/ a timeoutfactor, that timeout of
1200s is going to be multiplied by the timeout factor!

-- Jon

On 05/01/2017 01:44 PM, Brent Christian wrote:

> The test runs in question are using an increased timeout factor, but
> this test still needs more time than its current timeout provides.
>
> Thanks,
> -Brent
>
> On 5/1/17 1:22 PM, Roger Riggs wrote:
>> Hi,
>>
>> Also, should it not be the case that running with -Xcomp should increase
>> the timeout factor
>> to avoid such issues?  I'm not sure how that can be assured though.
>>
>> Roger
>>
>>
>> On 5/1/2017 3:47 PM, Brent Christian wrote:
>>> On 5/1/17 12:41 PM, Mandy Chung wrote:
>>>> Looks like this test execs a new VM for 66 times to exhaust the
>>>> combination of with and without security manager, with a valid or
>>>> malformed policy, client & custom loader in unnamed, named, automatic
>>>> module.
>>>
>>> Right.
>>>
>>>> I think we could take out the automatic module case as named
>>>> module would verify it.
>>>
>>> OK, I can do that.  I'm pretty sure an increase to the timeout will
>>> still be needed, though.
>>>
>>>> One refactor you can consider  by separating
>>>> them in several @run actions.  The timeout will apply to each action
>>>> but that does not help shorten the test execution time.
>>>
>>> I had the same thought, and concluded that without a reduction in
>>> overall execution time, the benefit of such a refactor is pretty
>>> limited.
>>>
>>> Thanks,
>>> -Brent
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
In reply to this post by Brent Christian-2
Brent Christian wrote:
 > Mandy Chung wrote:
 >>
 >> I think we could take out the automatic module case as named module
 >> would verify it.  One refactor you can consider  by separating them
 >> in several @run actions.
>> ...
>> I think it would make the test easier to read and understand the cases
>> it cover.
>
> True.  I'll see what I can do, then.

I've removed the test case for automatic modules, and added a @run
action for each policy file + system classloader configuration.  I also
removed the code to compile test sources, using @build instead.

I also made some (hopefully) clarifying changes to the code.

The String[] argument to execute() is now separated into
args/status/message, instead of being lumped into one String[], and then
having to be split() out of a single String.  Where execute() is called,
it's easier to read the args in order (versus assembled using
String.format()).

I also simplified the code for expected status/output, and made some
path variable names more descriptive.

Webrev:
http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/

Thanks,
-Brent
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
On 5/9/17 4:27 PM, Mandy Chung wrote:
>>
>> Webrev:
>> http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/
>
> Thanks for the clean up.  Does each @run action need 4 mins timeout?  Can it restore to the default timeout?

Yes.  Between the additional @runs and no longer testing automated
modules, I believe that should be fine.

-Brent
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
On 5/9/17 4:39 PM, Brent Christian wrote:
> On 5/9/17 4:27 PM, Mandy Chung wrote:
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/
>>
>> Can it restore to the default timeout?
>
> Yes.  Between the additional @runs and no longer testing automated
> modules, I believe that should be fine.

Webrev updated in place, with the timeout value removed.

-Brent

Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
Hi,

I have one more update, with a couple of suggested changes to simplify
the execute() calls:

* execute() takes a vararg, so explicit String[] creation can be omitted
(mostly).

* args common to every execute() call are consolidated into a List.
(The resulting arg reordering should not affect test execution.)

http://cr.openjdk.java.net/~bchristi/8177328/webrev.02/

Thanks,
-Brent

On 5/10/17 9:42 AM, Brent Christian wrote:

> On 5/9/17 4:39 PM, Brent Christian wrote:
>> On 5/9/17 4:27 PM, Mandy Chung wrote:
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/
>>>
>>> Can it restore to the default timeout?
>>
>> Yes.  Between the additional @runs and no longer testing automated
>> modules, I believe that should be fine.
>
> Webrev updated in place, with the timeout value removed.
>
> -Brent
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

Brent Christian-2
On 5/12/17 9:11 AM, Mandy Chung wrote:
>
> Minor comment:
>   95     private final List<String> COMMON_ARGS;
>
> This is an instance field and you can use lower case as the convention.
>
>  238                     return !s.equals("");
>
> You can consider using String::isEmpty.

Thanks, Mandy.  I will make these changes before pushing.

-Brent