RFR: 8222550: runtime/MemberName/MemberNameLeak.java times out

[David Holmes]

Hi Coleen,

On 18/04/2019 12:09 pm, coleen.phillimore at oracle.com wrote:
> 
> I didn't see the 02 change below.   I think the shouldContain function 
> should be added to output analyzer.  And as a utility patch, it should 
> be separate to help with backports that might need it.  So I chopped out 
> Stefan's function:
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8222713.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8222713

Metacomment: not sure why OutputAnalyzer should have a utility that 
takes an arbitrary file and searches it for strings? That's not part of 
the output that OutputAnalyzer is designed to analyze. This may be a 
useful utility but belongs elsewhere IMHO. The fact this doesn't 
reference any internal state of the OutputAnalyzer also suggests it 
should be a static utility method, not an instance method. Or define a 
new OutputAnalyzer constructor that takes a File and operates in its 
contents - though in that case you may be able to just convert the file 
contents to a String and use the existing OutputAnalyzer constructor 
that takes a String to start with.

Not clear what the expected semantics should be for searching for 
multiple lines. I would have expected to be searching for lines in the 
given order, but the code will match them in any order. That may be what 
you wanted, but it's not clear to me its what you'd always want. Needs 
to be documented either way.

+      * Verify that the contents of the file contains the given the set 
of strings

s/the set/set/

+         LinkedList<String> expectedList = new LinkedList<>();
+         for (String s : expectedStrings) {
+           expectedList.add(s);
+         }

List<String> expectedList = Arrays.asList(expectedStrings);

+         FileReader fr = new FileReader(file);
+         BufferedReader reader = new BufferedReader(fr);

These should be part of a try-with-resources block.

+         for (String line; (line = reader.readLine()) != null;) {

I'd find this cleaner as a while-loop:

String line;
while ((line = reader.readLine()) != NULL) {

Though I think there are simpler ways to deal with files - see 
java.nio.file.Files utility class.

> Leaving the MemberNameLeak.java change as:
> 
> open webrev at http://cr.openjdk.java.net/~coleenp/2019/8222550.01/webrev

Seems okay.

Thanks,
David
-----

> Tested with the new MemberNameLeak.java test.  hs tier1-3 testing in 
> progress.
> 
> All which look good to me.  Please review!
> 
> Thanks,
> Coleen
> 
> On 4/17/19 2:41 AM, Stefan Karlsson wrote:
>> On 2019-04-17 05:58, David Holmes wrote:
>>> Hi Stefan,
>>>
>>> On 17/04/2019 7:24 am, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch to fix a timeout in the MemberNameLeak test.
>>>>
>>>> https://cr.openjdk.java.net/~stefank/8222550/webrev.01/
>>>> https://bugs.openjdk.java.net/browse/JDK-8222550
>>>>
>>>> The test could fail if GCs happened during the setup phase when 
>>>> entries for all generated methods were created. When this happened 
>>>> the code to grow the table was triggered, which in turn cleaned out 
>>>> all so-far created entries.  This put the table in a condition where 
>>>> the grow / cleaning code didn't have to be triggered again. But the 
>>>> test still waited for it to happen. This patch adds all 
>>>> MethodHandles to an ArrayList, so that they are kept alive until 
>>>> it's time for them to be cleaned out. While debugging this timeout I 
>>>> added some extra logging. I've left it in the test in case we ever 
>>>> need to debug it again.
>>>
>>> Fix seems reasonable.
>>
>> Thanks.
>>
>>> A couple of comments:
>>>
>>>  119 "-Xlog:membername+table=trace,gc+verify=debug,gc",
>>>  120 
>>> "-Xlog:membername+table=trace,gc+verify=debug,gc:gc.%p.log:time,utctime,uptime,pid,level,tags", 
>>>
>>>
>>> I'm assuming you only actually want line 120?
>>
>> It was a quick and dirty way to get logging from 119 to the 
>> outputAnalyzer, and more comprehensive logging from line 120 saved to 
>> disk.
>>
>>>
>>> Is the log file copied across with the test artifacts in mach5? 
>>
>> Yes.
>>> I'm assuming you're using the file for gc logging so that the normal 
>>> test .jtr file is not inundated with excessive logging data.
>>
>> Yes, and because jtreg cuts in the middle of the output of tests with 
>> excessive logging.
>>
>> I created a more elaborate version that only logs to files, and 
>> perform the verification on those files:
>>  https://cr.openjdk.java.net/~stefank/8222550/webrev.02.delta
>>  https://cr.openjdk.java.net/~stefank/8222550/webrev.02
>>
>> Thanks,
>> StefanK
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> Testing: tier1-3 and multiple tier1_runtime runs on osx where the 
>>>> timeouts reproduced.
>>>>
>>>> The patch is applied on top of the patch in:
>>>> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-April/033820.html 
>>>>
>>>>
>>>> Thanks,
>>>> StefanK
>>
>