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

[Yekaterina Kantserova]

Staffan,

The webrev with changes can be found here:
http://cr.openjdk.java.net/~ykantser/7185591/webrev.02/

If it looks good, could you please be my sponsor and push the changes? 
The patch is attached to this mail.

Thanks,
Katja



On 01/15/2014 11:37 AM, Staffan Larsen wrote:
> 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 
> <mailto: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/ 
>> <http://cr.openjdk.java.net/%7Eykantser/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 
>>> <mailto: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/ 
>>>> <http://cr.openjdk.java.net/%7Eykantser/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/fe93a007/attachment-0001.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 7185591.2.patch
Type: text/x-patch
Size: 37773 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140115/fe93a007/7185591.2-0001.patch