RFR: 8203491: [TESTBUG] Port heapdump tests into java

Tue Jun 12 23:38:37 UTC 2018

Looks good.
Coleen

On 6/12/18 7:13 PM, Leonid Mesnik wrote:
> You are right. Executing these in tier1 is not a very good idea.
> I excluded them from tier1_serviceability so they are run in 
> tier3_runtime with all other serviceability tests.
>
> The new webrev:
> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.03/ 
> <http://cr.openjdk.java.net/%7Elmesnik/8203491/webrev.03/>
>
>
> The only updated file is TEST.groups with new SA tests excluded from 
> tier1:
>   #############################################################################
> --- old/test/hotspot/jtreg/TEST.groups	2018-06-12 16:09:27.605104992 -0700
> +++ new/test/hotspot/jtreg/TEST.groups	2018-06-12 16:09:27.232105998 -0700
> @@ -209,6 +209,7 @@
>    -runtime/ConstantPool/IntfMethod.java \
>    -runtime/ErrorHandling/CreateCoredumpOnCrash.java \
>    -runtime/ErrorHandling/ErrorHandler.java \
> + -runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java \
>    -runtime/ErrorHandling/TimeoutInErrorHandlingTest.java \
>    -runtime/logging/MonitorMismatchTest.java \
>    -runtime/memory/ReserveMemory.java \
> @@ -271,6 +272,8 @@
>     serviceability/logging \
>     serviceability/sa \
>     -serviceability/sa/ClhsdbScanOops.java \
> +  -serviceability/sa/TestJmapCore.java \
> +  -serviceability/sa/TestJmapCoreMetaspace.java \
>     -serviceability/sa/TestHeapDumpForLargeArray.java
> Leonid
>
>> On Jun 12, 2018, at 3:30 PM, coleen.phillimore at oracle.com 
>> <mailto:coleen.phillimore at oracle.com> wrote:
>>
>> This cleanup looks fine.  Do the test/hotspot/jtreg/serviceability/sa 
>> test run in tier1?  Should they be excluded also?
>> thanks,
>> Coleen
>>
>> On 5/25/18 12:55 PM, Leonid Mesnik wrote:
>>> Jini
>>>
>>> Thank you for review. I added final.
>>>
>>> Still waiting for review from 'R'eviewer and from runtime.
>>>
>>> Leonid
>>>
>>>> On May 24, 2018, at 11:31 PM, Jini George <jini.george at oracle.com 
>>>> <mailto:jini.george at oracle.com>> wrote:
>>>>
>>>> The SA tests look good to me. One minor comment.
>>>>
>>>>    public static String HEAP_OOME = "heap";
>>>>    public static String METASPACE_OOME = "metaspace";
>>>>
>>>> These could be declared as public static final String.
>>>>
>>>> Thanks,
>>>> Jini.
>>>>
>>>>
>>>>
>>>> On 5/25/2018 4:37 AM, Leonid Mesnik wrote:
>>>>> Hi
>>>>> Thanks for feedback. It is a good idea.
>>>>> Split TestHeapDumpOnOutOfMemoryError into 3 tests. The only test 
>>>>> TestHeapDumpOnOutOfMemoryErrorInMetaspace.java requires 
>>>>> timeout=240 and excluded from tier1 now.
>>>>> Also I fixed parameters for TestHeapDumpPath test and removed 
>>>>> unneeded '@modules java.base/jdk.internal.misc' from all tests.
>>>>> New full webrev:
>>>>> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.02/ 
>>>>> <http://cr.openjdk.java.net/%7Elmesnik/8203491/webrev.02/>
>>>>> inc changes
>>>>> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.02-01/
>>>>> Leonid
>>>>>> On May 24, 2018, at 2:08 PM, Igor Ignatyev 
>>>>>> <igor.ignatyev at oracle.com <mailto:igor.ignatyev at oracle.com>> wrote:
>>>>>>
>>>>>> 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.mesnik at oracle.com <mailto:leonid.mesnik at 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.george at oracle.com <mailto:jini.george at 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.
>>
>