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

Tue Jun 12 23:13:35 UTC 2018

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/~lmesnik/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 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> 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/
>>>> 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.
> 

