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

[David Holmes]

On 18/04/2019 5:31 pm, Stefan Karlsson wrote:
> 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/

We don't need the Utils function

Utils.fileAsString(file.toString())

  since JDK 11 we can just use:

Files.readString(file)

Cheers,
David
-----



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