Hi Leonid,

I haven't reviewed your change fully, although I noticed that you merged 
several tests into one -- TestHeapDumpOnOutOfMemoryError, I don't think it's a 
good idea as we lose atomicity of test results/executions. could you please 
split TestHeapDumpOnOutOfMemoryError into independent tests?

-- Igor

> On May 24, 2018, at 10:54 AM, Leonid Mesnik <leonid.mes...@oracle.com> wrote:
> 
> Hi
> 
> Found new webrev here:
> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.01/ 
> <http://cr.openjdk.java.net/~lmesnik/8203491/webrev.01/>
> and inc diff
> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.01-00/ 
> <http://cr.openjdk.java.net/~lmesnik/8203491/webrev.01-00/>
> 
> I don't know if existing 64m is enough to produce a lot of classes. However 
> this size was used in original test so I use same in new test 
> TestJmapCoreMetaspace.java.
> 
> I fixed comments, import and timeout(set to 240) also.
> 
> Leonid
> 
>> On May 24, 2018, at 9:16 AM, Jini George <jini.geo...@oracle.com> wrote:
>> 
>> Hi Leonid,
>> 
>> My comments inline.
>> 
>> On 5/24/2018 12:09 AM, Leonid Mesnik wrote:
>> 
>>> I am not sure that JMapMetaspaceCore provides any additional coverage. The 
>>> test just fill 64M of metaspace and then send signal to dump core. So I 
>>> don't see how this test could improve coverage.
>>> I think that idea of original test was to fill PermGen like Heap to expand 
>>> it as much as possible or it was just an analog of test 
>>> OnOOMToFileMetaspace. While current test just fill highly limited 
>>> metaspace. So number of classes seems to be not significantly larger then 
>>> for current TestJmapCore.java test. From my point of view it would be make 
>>> a sense to generate dump containing a lot of loaded classes or some very 
>>> large classes.
>>> While current test looks pretty similar to existing TestJmapCore.java test. 
>>>  Please let me know if you see the test scenario when such test could be 
>>> useful.
>> 
>> From what I can make out, EatMemory with -metaspace would create a lot of 
>> loaded classes with GeneratedClassProducer. And this could provide some good 
>> testing for writeClassDumpRecords() of HeapHprofBinWriter. Let me know if 
>> you think I have overlooked something.
>> 
>> 
>>>> * You might want to increase the timeout factor for this test. The test 
>>>> times out on my machine.
>>>> 
>>>> 
>>> I see that test finishes in 1 minute in our lab while. I see that it takes 
>>> 30 seconds on 2CPU Oracle Linux VM with 2GB java heap. And test just fails 
>>> with JDK-8176557 when I increase heap.
>>> How many time it takes on you machine? The timeoutFactor might be used for 
>>> untypical environment/command-line options.
>> 
>> It took about 130 secs a couple of times. Don't know if it was an anomaly.
>> 
>>>> * You might want to consider removing the corefile and the heapdump files 
>>>> after the test execution (in the cases where the test passes).
>>>> 
>>> The default jtreg retain policy in make files just removes all files in 
>>> test directory for passed tests. The jtreg default test policy says
>>> "If -retain is not specified, only the files from the last test executed 
>>> will be retained".
>>> So it should be not a problem in most of cases. While there is no way for 
>>> user to retain core/heapdump files even if user wants to keeps them.
>> 
>> Ok.
>> 
>>> However if it is the common rule for sa tests to delete such artifacts then 
>>> I could remove heap/core dumps.
>>>> 
>>>> One suggestion is to check if /proc/sys/kernel/core_pattern has a line 
>>>> starting with '|' and print a warning that a crash reporting tool might be 
>>>> used (Something like in ClhsdbCDSCore.java). But it is just a suggestion 
>>>> and you are free to ignore it. In due course, we could include this test 
>>>> also as a part of the consolidation of SA's corefile testing effort 
>>>> (https://bugs.openjdk.java.net/browse/JDK-8202297).
>>>> 
>>> I would prefer to left this improvement for JDK-8202297. I think good core 
>>> dump processing/ulimit settings requires more efforts and testing and 
>>> different version of Linux. (Might be even for Non-Oracle platforms).
>>> Also logic in test ClhsdbCDSCore.java is slightly different. It tries to 
>>> get possible core location from hs_err file and print this hint of core 
>>> file from hs_err doesn't exists. While printing this hint if core dumps are 
>>> just completely disabled might just confuse users.
>> Sounds fine.
>> 
>> Thanks,
>> Jini.
> 

Reply via email to