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.geo...@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.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. >>>> >>>
