RFR (L): 8068685 - [TESTBUG] Ensure @library works in jtreg -agentvm mode

[David Holmes]

Hi George,

On 25/02/2015 11:42 PM, George Triantafillou wrote:
> Hi David,
>
> Thanks for your review.  Comments inline:
>
> On 2/24/2015 11:55 PM, David Holmes wrote:
>> On 25/02/2015 6:45 AM, George Triantafillou wrote:
>>> Please review this fix for JDK-8068685:
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8068685
>>> webrev: http://cr.openjdk.java.net/~gtriantafill/8068685/webrev.00
>>> <http://cr.openjdk.java.net/%7Egtriantafill/8068685/webrev.00>
>>>
>>> "@build com.oracle.java.testlibrary.*" was added to all relevant tests,
>>
>> I thought we had an RFE open against jtreg to avoid the need to do
>> this, but it seems I was mistaken. :(
>>
>> Most of the additions to @build seem okay. A few minor issues:
>>
>> --- old/test/compiler/whitebox/ForceNMethodSweepTest.java 2015-02-24
>> 10:31:03.991071066 -0800
>> +++ new/test/compiler/whitebox/ForceNMethodSweepTest.java 2015-02-24
>> 10:31:03.617052982 -0800
>> @@ -35,6 +35,9 @@
>>   * @test
>>   * @bug 8059624 8064669
>>   * @library /testlibrary /../../test/lib
>> + * @build com.oracle.java.testlibrary.* ForceNMethodSweepTest
>> + * @ignore 8066998
>> + * @library /testlibrary /../../test/lib
>>   * @build ForceNMethodSweepTest
>>
>> The second @library shouldn't have been added.
> Good catch, thanks.  Changed.
>
>>
>> --- old/test/gc/g1/TestStringSymbolTableStats.java    2015-02-24
>> 10:32:28.209143019 -0800
>> +++ new/test/gc/g1/TestStringSymbolTableStats.java    2015-02-24
>> 10:32:27.855125901 -0800
>> @@ -22,11 +22,13 @@
>>   */
>>
>>  /*
>> - * @test TestStringSymbolTableStats.java
>> + * @test TestStringSymbolTableStats
>>
>> Why did you change the @test? I see a lot of unchanged @test foo.java.
> According to the jtreg tag spec
> http://openjdk.java.net/jtreg/tag-spec.html, either way is correct.  The
> only tests that were considered for this change included "@library
> /testlibrary", so I changed this one for consistency with other tests
> that I've written.  I'll change it back if it's a concern.

No big deal but caused confusion when reviewing.

>>> which resulted in longer test execution times.  In order to offset these
>>> longer test execution times and to support running jtreg in -agentvm
>>> mode, the following changes were made:
>>>
>>> test/Makefile: added -agentvm option
>>
>> I'm unclear exactly what this buys us - do we just save on the startup
>> costs of new vms running jtreg? I thought the default was samevm anyway?
>>
>> Also does this mean that all VMs will share a single copy of the
>> testlibrary or will we still have a copy per test that gets built each
>> time?
> The jtreg command help http://openjdk.java.net/jtreg/command-help.html
> doc states:
>
> -agentvm "Run tests using a pool of reusable JVMs."
> -samevm "If possible, run each test in the same JVM as the JavaTest"
>
> I'm not really sure how samevm decides "if possible", but the intent of
> this change was to explicitly add "@build com.oracle.java.testlibrary.*"
> to relevant jtreg runtime tests without increasing test execution
> times.  The analysis below shows decreased test execution times using
> agentvm.

Yes but I'd like to understand why that is the case, and whether we are 
still being inefficient in this even if better than the initial attempt. 
It is very unclear to me how the addition of agentvm is influencing the 
building of the testlibrary.

>>
>>> test/testlibrary/com/oracle/java/testlibrary/ProcessTools.java: added
>>> -classpath since -agentvm passes the classpath differently than -othervm
>>>
>>> *Testing shows an 11% speed improvement for JPRT execution of the
>>> hotspot testset, and a 48% speed improvement for
>>> hotspot_runtime*/gc*/compiler*/serviceability* tests.**
>>>
>>> *Note that some tests required the explicit addition of -othervm in
>>> order for the tests to pass.  In addition, a number of gc, compiler and
>>> runtime tests were modified with @run so they would run successfully
>>> with -agentvm.  Consequently, reviews from members of the compiler, gc,
>>> runtime, and serviceability teams would be very helpful.
>>
>> I'm unclear about the need for some of the othervm changes. I would
>> only expect othervm to be needed if passing -XX options via @run ??
> I added @run to some tests, and some of them failed with -agentvm until
> I added -othervm.

This is a puzzle - perhaps a jtreg bug. Without the @build for the 
testlibrary you can use -agentvm without a problem based on a few 
experiments I did. Once you add the @build you then also have to add 
@run, but the simple: @run test fails with e.g.:

test result: Error. Bad action for script: CdsSameObjectAlignment

However it suffices to add "main" in the @run:

@run main CdsDifferentObjectAlignment

without needing to add /othervm

Cheers,
David


> -George
>>
>> Thanks,
>> David
>>
>>> Thanks to Erik Helin for providing a solution for passing classpath with
>>> -agentvm.
>>>
>>> The fix was tested locally on Linux with jtreg and on all platforms with
>>> the JPRT hotspot testset.*
>>> *
>>> Thanks.
>>>
>>> -George
>>>
>