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

[Stefan Karlsson]

On 2019-04-18 06:26, David Holmes wrote:
> 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

Yes, this is exactly what I would have expected.

  - 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.


I would rather have the OutputAnalyzer constructor do it for me, instead 
of having to do this conversion every time I want to analyze a file.


> 
> 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.

If we create a OutputAnalyzer(File) constructor, we don't have to care 
about that.

I've added that function:
  http://cr.openjdk.java.net/~stefank/8222713/webrev.01/

and updated the webrev for the original bug:
  https://cr.openjdk.java.net/~stefank/8222550/webrev.03.delta/
  https://cr.openjdk.java.net/~stefank/8222550/webrev.03/

Thanks,
StefanK

> 
> +      * 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
>>>
>>