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

Thu Apr 18 08:16:09 UTC 2019


On 2019-04-18 09:55, David Holmes wrote:
> 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)

Updated:
  http://cr.openjdk.java.net/~stefank/8222713/webrev.02.delta/
  http://cr.openjdk.java.net/~stefank/8222713/webrev.02/

Thanks,
StefanK

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