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
<[email protected]> 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
>> <[email protected]> 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
