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

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8203491: [TESTBUG] Port heapdump tests into java
From:       Leonid Mesnik <leonid.mesnik () oracle ! com>
Date:       2018-05-25 16:55:46
Message-ID: 65A5565D-AAF3-44C9-81EC-1BA5A52B5B55 () oracle ! com
[Download RAW message or body]

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@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@oracle.com \
> > > <mailto:igor.ignatyev@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@oracle.com \
> > > > <mailto:leonid.mesnik@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@oracle.com \
> > > > > <mailto:jini.george@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.
> > > > 
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic