Hello, I think "binary crash file" is not a clear wording, it made me wonder if this is a hprof heap dump. It should explicitely say "sytem core dump, when enabled". (The webrev below did not contain actual crash-code, is it already commited?).
BTW, rejecting large array allocations could be argued that it is not a situation where you want/need the exit. That might not be the best test case? I agree with Abort* as the better naming Gruss Bernd -- http://bernd.eckenfels.net -----Original Message----- From: cheleswer sahu <cheleswer.s...@oracle.com> To: David Holmes <david.hol...@oracle.com>, serviceability-dev@openjdk.java.net, Staffan Larsen <staffan.lar...@oracle.com> Cc: Rajendrakumar Pallath <rajendrakumar.pall...@oracle.com> Sent: Do., 03 Dez. 2015 8:28 Subject: Re: RFR[ 9u-dev] JDK-8138745: Implement ExitOnOutOfMemory and CrashOnOutOfMemory in HotSpot Hi, Thanks David and Staffan for your comments. Please review the code changes in the updated webrev below http://cr.openjdk.java.net/~kevinw/8138745/webrev.01/ Regards, Cheleswer On 11/26/2015 11:58 AM, David Holmes wrote: > Sorry forgot the tests .... > > test/runtime/ErrorHandling/TestExitOnOutOfMemoryError.java > > This test is checking that new Object[Integer.MAX_VALUE]; caused the > "Requested array size exceeds VM limit" failure _but_ it doesn't > actually do anything to verify that the VM terminated because of the > ExitOnOutOfMemory flag. I suggest: > > a) augment the termination message in the VM as I suggested earlier so > that you can be sure you hit the termination code > b) check for a zero/non-zero return code as appropriate > c) Add: throw new Error("OOME was not triggered"); after line 41. > d) Put a try/catch(OOME) around the allocation and throw an Error if > you get to the catch block > > That way we will get a test failure when Arrays 2.0 allows such > massive arrays to be created :) > > Similar considerations for TestCrashOnOutOfMemoryError.java, but you > also need to disable core dump generation. > > David > ----- > > On 26/11/2015 3:43 PM, David Holmes wrote: >> Hi, >> >> On 25/11/2015 10:40 PM, cheleswer sahu wrote: >>> Hi, >>> >>> Please review the code changes for >>> "https://bugs.openjdk.java.net/browse/JDK-8138745";. >>> Web review link: >>> <http://cr.openjdk.java.net/%7Ekevinw/8138745/webrev.00/>http://cr.openjdk.java.net/~kevinw/8138745/webrev.00/ >>> >>> >>> >>> >>> Enhancement Brief: >>> ExitOnOutOfMemoryError: When user enable this option, the JVM exits on >>> the first occurrence of an out-of-memory error. It can be used if user >>> prefer restarting an instance of the JVM rather than handling out of >>> memory errors. >>> >>> CrashOnOutOfMemoryError: If this option is enabled, when an >>> out-of-memory error occurs, the JVM crashes and produces text and >>> binary >>> crash files. >> >> The term "crash" is not very appropriate - crashes are bad things. Abort >> may have been a better choice. >> >>> For more details please refer http://ccc.us.oracle.com/8138745 >> >> This is not accessible outside of Oracle. >> >> A few minor comments: >> >> src/share/vm/runtime/globals.hpp >> >> + "JVM crashes and produces text and binary crash files") >> >> Terminology should be consistent with other options that control core >> dump. Should also say "on first occurrence of an out-of-memory error". >> >> src/share/vm/utilities/debug.cpp: >> >> 308 if (CrashOnOutOfMemoryError) { >> 309 tty->print_cr("java.lang.OutOfMemoryError: %s", message); >> 310 fatal("OutOfMemory encountered: %s", message); >> 311 } >> >> don't really need the tty->print when using the fatal. Though you may >> want to use the j.l.OOME form of the message for consistency. Might also >> be useful for both Crash and Exit to include in the logged messages the >> fact that these flags were involved - something like: >> >> "Terminating due to java.lang.OutOfMemoryError: %s" >> "Aborting due to java.lang.OutOfMemoryError: %s" >> >> Thanks, >> David >> ----- >> >>> >>> Regards, >>> Cheleswer >>>
