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

Thu Apr 18 11:26:04 UTC 2019

This looks great!
thanks,
Coleen

On 4/18/19 4:16 AM, Stefan Karlsson wrote:
>
>
> 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
>>>>>>
>>>>>