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

[David Holmes]

One correction ...

On 26/02/2015 12:16 PM, David Holmes wrote:
> 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

User error. You must have an action, such as main, after @run.

David

> 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
>>>>
>>