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.ignat...@oracle.com
<mailto:igor.ignat...@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.mes...@oracle.com
<mailto: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
<mailto: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.
