RFR: 8222550: runtime/MemberName/MemberNameLeak.java times out
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Apr 18 10:10:07 UTC 2019
Thanks!
In the interest of getting this fixed before the holidays, I intend to
push this as soon as the last round of testing passes.
StefanK
On 2019-04-18 11:13, David Holmes wrote:
> Works for me :)
>
> Thanks,
> David
>
> On 18/04/2019 6:16 pm, 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
>>>>>>>
>>>>>>
More information about the hotspot-runtime-dev
mailing list