RFR (S): 7185591: jcmd-big-script.sh ERROR: could not find app's Java pid.

[Staffan Larsen]

Katja,

test/lib/testlibrary/jdk/testlibrary/JcmdBase.java
The toolArgs parameter should be renamed jcmdArgs.

test/lib/testlibrary/jdk/testlibrary/OutputAnalyzer.java
423     public int indexOf(String pattern) {
You don’t have to convert the List to an array before iterating through it. Use lLines.get(i).matches(…) instead.

470     public int shouldMatchByLine(String from, String to, String pattern) {
This method still reads the lines into an ArrayList (at most) three times. It calls asLines() once and indexOf() twice. There could be a version of indexOf() that takes a List<String>. Actually indexOf could be private and always take a List<String>.


Otherwise ok!

/Staffan



On 15 jan 2014, at 10:13, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:

> Staffan, thank you for pointing out performance and test.src issues!
> 
> The webrev with fixes can be found here:
> http://cr.openjdk.java.net/~ykantser/7185591/webrev.01/
> 
> Please, see my comments in-lined.
> 
> Thanks,
> Katja
> 
> 
> 
> On 01/13/2014 01:19 PM, Staffan Larsen wrote:
>> Katja,
>> 
>> test/lib/testlibrary/jdk/testlibrary/JcmdBase.java
>>  68      * Run jcmd standalone
>> 
>> I think you should expand a bit on what “standalone” means here. It took me a while to understand the difference.
> Fixed.
>> 
>> test/lib/testlibrary/jdk/testlibrary/OutputAnalyzer.java
>> 423     public int indexOf(String pattern) {
>> 
>> This seems very inefficient. Add all lines to an ArrayList and then walk through them one at a time to if it matches and then walk through them once again to find the index of that line.
>> 
>> 469     public int shouldMatchByLine(String from, String to, String pattern) {
>> 
>> Same inefficiency here, but worse because both asLines() and indexOf() does the same work.
>> 
>> test/lib/testlibrary/jdk/testlibrary/Utils.java
>> 65     public static final String TEST_SRC = System.getProperty("test.src").trim();
> Fixed.
>> 
>> I wonder if this really works. Isn’t “test.src” different for different tests? A property that jtreg changes
> Yes, it is different.
>> before invoking each test? Or does this work because each test is run in a different class loader and Utils.java will exist once in each class loader?
> I've learned now it works because jtreg compiles all classes a test needs to a separate location. For example when I run TestJcmdDefaults there will be both TestJcmdDefaults.class and jdk/testlibrary/Utils.class under /tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/. The class loader will load Utils for TestJcmdDefaults from /tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/.
> 
> ...
> [Loaded jdk.testlibrary.Utils from file:/tmp/jtreg/jtreg-workdir/classes/sun/tools/jcmd/]
> ...
> 
> But I think it's better to declare TEST_SRC per test precisely because it's different for different tests.
> 
>> 
>> 
>> /Staffan
>> 
>> 
>> On 10 jan 2014, at 13:50, Yekaterina Kantserova <yekaterina.kantserova at oracle.com> wrote:
>> 
>>> Hi,
>>> 
>>> Could I please have a review of this fix.
>>> 
>>> In this fix I've rewritten sun/tools/jcmd/* tests in pure java to get rid of all intermittent failures depending on for example MKS or race conditions (test application has not yet started when the test start to run).
>>> 
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~ykantser/7185591/webrev.00/
>>> 
>>> Primal bug:
>>> https://bugs.openjdk.java.net/browse/JDK-7185591
>>> 
>>> Similar bugs:
>>> https://bugs.openjdk.java.net/browse/JDK-6977426
>>> https://bugs.openjdk.java.net/browse/JDK-8020798
>>> https://bugs.openjdk.java.net/browse/JDK-7130656 (this one is blocked by https://bugs.openjdk.java.net/browse/JDK-8031482 so far)
>>> 
>>> Thanks,
>>> Katja

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140115/c7e6cdc0/attachment.html