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

Wed May 23 18:39:17 UTC 2018

Hi

Thank you for review and feedback. See my comments inline. 
I am going to get more feedback and then send new version with fixes for your comments. 

> On May 23, 2018, at 8:48 AM, Jini George <jini.george at oracle.com> wrote:
> 
> Hi Leonid,
> 
> I have mostly looked at only the following file:
> 
> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJmapCore.java.html
> 
> * Would you be adding JMapMetaspaceCore testing to this? Other than this, I think the jhsdb heapdump testing from vmTestbase has been covered.
> 
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.

> * 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.

> * 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.

However if it is the common rule for sa tests to delete such artifacts then I could remove heap/core dumps.

> * The following imports:
> import jdk.test.lib.classloader.GeneratingClassLoader;
> import java.util.Arrays;
> import java.util.stream.Collectors;
> 
> are not needed.
> 
Thanks, will fix it.
> * Nit:
> // If any arguments are set prints pid so main process coud find corefile
> 
> coud --> could
> 
> * "Has not been able to find coredump. Test skipped."
Thanks.
> 
> 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.

Leonid

> 
> > I have verified that test  still could reproduce issue
> > https://bugs.openjdk.java.net/browse/JDK-8176557 if default heap size is
> > large enough. However I think that it make a sense to add variant of
> > this test which explicit heap settings as a regression fix for bug
> > JDK-8176557 <https://bugs.openjdk.java.net/browse/JDK-8176557>.
> >
> 
> Sounds good to me.
> 
> > Also I haven't managed to reproduce following issues with neither
> > existing or new tests:
> > https://bugs.openjdk.java.net/browse/JDK-8001227
> > https://bugs.openjdk.java.net/browse/JDK-8023376
> > https://bugs.openjdk.java.net/browse/JDK-8051445
> > Corresponding  tests were excluded for a long time so there are no any
> > results for them. I also see comments in bugs with unsuccessful attempts
> > to reproduce issues.  I think that it makes a sense just to remove these
> > entries from problemlist and file new open bugs if any of failures happen.
> 
> Sounds good to me.
> 
> Thanks,
> Jini (Not a Reviewer).
> 
> 
> On 5/23/2018 6:45 AM, Leonid Mesnik wrote:
>> Hi
>> Could you please review following patch which rewrite vmTestbase/heapdump tests as a jtreg java tests.
>> These heapdump tests were developed as a shell tests which verify heapdump functionality in JDK 5. They work fine but needs some refactoring.
>> webrev: http://cr.openjdk.java.net/~lmesnik/8203491/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8203491
>> Below is high-level overview of existing tests and changes:
>> 1) Tests vmTestbase/heapdump/OnOOM* verify that VM correctly generates heapdump when OOME in heap or metaspace happens. Also they verify that -XX:HeapDumpPath= works correctly.
>> Re-written as:
>> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.00/test/hotspot/jtreg/runtime/ErrorHandling/TestHeapDumpOnOutOfMemoryError.java.html
>> Test provokes OOME and verifies generated heapdump.
>> Please note that new jtreg test is very similar and use same test patterns as most of ErrorHandling tests in http://hg.openjdk.java.net/jdk/jdk/file/tip/test/hotspot/jtreg/runtime/ErrorHandling
>> The main goal is to verify error handling when OOME is happen so I used the same way to provoke OOME as most of other tests in this directory.
>> New test might takes more then minute to complete test so I removed from tier1.
>> 2) Tests vmTestbase/heapdump/JMap* verify that jmap correctly generates heapdump from live process and core/mdmp file.
>> There are existing tests which verify that jmap could correctly dump heap for live process:
>> http://hg.openjdk.java.net/jdk/jdk/file/f4735ff8d17d/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForInvokeDynamic.java
>> http://hg.openjdk.java.net/jdk/jdk/file/f4735ff8d17d/test/hotspot/jtreg/serviceability/sa/TestHeapDumpForLargeArray.java
>> I believe that they provide the same coverage as heapdump/JMapHeap and heapdump/JMapMetaspace tests and no new tests is needed.
>> The new test which verifies that jmap generate heapdump for corefile is:
>> http://cr.openjdk.java.net/~lmesnik/8203491/webrev.00/test/hotspot/jtreg/serviceability/sa/TestJmapCore.java.html
>> Test provokes OOME to generate corefile (or mdml), generate heapdump with jhsdb jmap and verify it. Previously tests send signals to provoke coredump. Since JDK9 option 'CreateCoredumpOnCrash' could be used to easily dump core on all platforms. Please note that test silently pass if it doesn't find core file. Even if test tries to set ulimit -c unlimited there is no guarantee that core file is generated and could be found.
>> I have verified that test  still could reproduce issue https://bugs.openjdk.java.net/browse/JDK-8176557 if default heap size is large enough. However I think that it make a sense to add variant of this test which explicit heap settings as a regression fix for bug JDK-8176557 <https://bugs.openjdk.java.net/browse/JDK-8176557>.
>> Also I haven't managed to reproduce following issues with neither existing or new tests:
>> https://bugs.openjdk.java.net/browse/JDK-8001227
>> https://bugs.openjdk.java.net/browse/JDK-8023376
>> https://bugs.openjdk.java.net/browse/JDK-8051445
>> Corresponding  tests were excluded for a long time so there are no any results for them. I also see comments in bugs with unsuccessful attempts to reproduce issues.  I think that it makes a sense just to remove these entries from problemlist and file new open bugs if any of failures happen.
>> I quickly looked through all resolved bugs  found by heapdump/ tests. Most of them are related with different size limits overflow.  It might be make a sense also create test which fill heap with large amount of small objects to possible overflow of sizes of records. The similar functionality was removed by JDK-8144137 <https://bugs.openjdk.java.net/browse/JDK-8144137>.  But I would prefer to have separate RFE since it is not supported by existing tests.
>> Please let me know if it is possible to lost test coverage during this conversion and more tests are needed. The more conservative way might be to left existing tests for some time to see if if they still found different bugs.
>> Testing:
>> Run new tests on linux-x64,windows-x64,macos-x64.
>> Run tier1/2.
>> Leonid

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180523/8c228368/attachment.html>