Martin,
I agree regarding the bug - this fix provides a consistent approach to
all situations where an OOME is thrown. (You can consider this an
additional review.)
My meta-question is whether OOME should have been thrown for this case:
it somehow seems wrong to me - I would not handle this error in the same
way I would handle a true OOM, because this is obviously a simple
programming error - but other than throwing an unspecified
RuntimeException I don't see a better alternative. And it's a little
late to change this now.
My caution with the fix is that it will now post JVMTI events in
circumstances where they were not previously posted. I doubt any real
code will be affected by this, but it is possible that there is a test
somewhere that may get upset by it. You know how sensitive some of our
tests can be. ;)
Cheers,
David
Martin Buchholz said the following on 06/15/09 08:43:
I'm far from an expert on jvmti or hotspot's error handling, but...
If I've asked for a command to be run on OOME,
having it not run feels very much like a bug.
I agree that OOME might not perfectly match the situation,
but is there a better course of action?
I don't see any reasonable alternative to throwing OOME.
Martin
On Sun, Jun 14, 2009 at 15:25, David Holmes - Sun Microsystems
<[email protected] <mailto:[email protected]>> wrote:
Martin, Jeremy,
This change has been bugging me. I guess what I don't like is not
the "fix" but the fact that we report OutOfMemoryError for this
situation in the first place! We are not out-of-memory! We've been
asked to allocate something that is impossible to allocate given the
physical constraints of the memory system. The heap could actually
be nearly empty! If I were executing a OOM handler using the
"onError" mechanism I'm not sure I'd want it to run for this case.
This fix makes this usage consistent with all the other OOM
situations, but we post JVMTI resource exhausted events when the
resource need not be exhausted at all! I'm not sure that is the
right thing to do. Even assuming it is the right thing to do, this
may impact some tests as it now generates additional JVMTI events.
David Holmes
Martin Buchholz said the following on 06/15/09 05:37:
I've polished the changes in preparation for committing.
I'll commit once I have reviewer approval.
You'll need to let me know which forest to commit to.
Test webrev is now at:
http://cr.openjdk.java.net/~martin/jvmti-oom-test/
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom-test/>
Now has more tests and filled-in @bug line.
Hotspot fix webrev is at:
http://cr.openjdk.java.net/~martin/jvmti-oom/
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom/>
I've hacked on my private copy of webrev to make the output more
suitable for
external contributors.
I think it's time to again beg the hotspot integrators
to be sure to run the java/lang and java/util/concurrent tests
from the jdk repo before committing changes to MASTER.
Martin
On Sat, Jun 13, 2009 at 10:18, Tim Bell <[email protected]
<mailto:[email protected]> <mailto:[email protected]
<mailto:[email protected]>>> wrote:
Martin Buchholz wrote:
I've called my own bluff and implemented a test case for this
http://cr.openjdk.java.net/~martin/jvmti-oom/
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom/>
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom/>
Jeremy's original fix is in this hotspot webrev:
http://cr.openjdk.java.net/~martin/jvmti-oom-hotspot/
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom-hotspot/>
<http://cr.openjdk.java.net/%7Emartin/jvmti-oom-hotspot/>
Sun folks (Tim?), please take up the process issues:
- please review test and fix
- file one (or two?) "real" bugs or
For the HotSpot VM side:
6850957 hotspot/jvmti JVMTI OOM handling when arrays /
objects
are too large
For the test case:
6850958 java/classes_lang JVMTI OOM handling when arrays /
objects are too large
It's non-traditional to have fixes cross the hotspot/jdk
barrier,
but this was the easiest way to write a test case.
This happens most often in the Serviceability area, for
example when
fixes hit JVM TI and JDWP code. You have another good
example, where
the most natural test case fits in
JDK/test/java/lang/ProcessBuilder
The parent bugzilla report is:
https://bugs.openjdk.java.net/show_bug.cgi?id=100067
I filed two internal bug reports that should be visible on
bugs.sun.com <http://bugs.sun.com> <http://bugs.sun.com>
in a few working days. Using URL surgery, I predict the URLs
will be:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6850957
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6850958
Later today I will set up a forest, apply the patches from the
webrevs, and send it through JPRT. I want to see the HotSpot
test results, as well as the new ProcessBuilder/Basic.java
Tim
