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

[Yekaterina Kantserova]

Staffan,

I've missed to rename toolArgs in JcmdBase.java.

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

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 03:30 PM, Yekaterina Kantserova wrote:
> 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/20140116/ab962dac/attachment-0001.html 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 7185591.3.patch
Type: text/x-patch
Size: 37844 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140116/ab962dac/7185591.3-0001.patch