I'm not sure what potential refactoring was being referred to given that
this new fix simply adds a few calls to report_java_out_of_memory()
prior to throwing the OOME. On that front the actual fix looks fine to
me - it adds the "OnOOME" handling without messing with JVMTI at all
(which means the JVMTI mention in the bug synopsis is somewhat misleading.)
As far as the test goes, yes it should be in hotspot as a jtreg test but
given you have to check the output of a exec'ed error program I don't
know how to configure that either. :(
One issue with the test however: there were four changes made to the VM
code, but there are only three test cases! Which one is missing?
And as Alan suggested, as I'm not an official runtime member these days,
someone from runtime should also "rubber stamp" this.
Thanks,
David
Martin Buchholz said the following on 06/28/09 09:58:
Alright, I have a new simple version of the hotspot part of the
jvmti-oom fix that should make Alan happy.
...Except for the usual problem that the code is screaming
for a bit of refactoring, and it's not quite clear what file
and function name it should be refactored to. I'll do the
easy refactoring if you give me the names to use.
Or simply give me thumbs up and I will commit.
http://cr.openjdk.java.net/~martin/jvmti-oom/
http://cr.openjdk.java.net/~martin/jvmti-oom-test/
Thanks,
Martin
On Wed, Jun 24, 2009 at 01:15, Alan Bateman <alan.bate...@sun.com
<mailto:alan.bate...@sun.com>> wrote:
David Holmes - Sun Microsystems wrote:
Jeremy,
As I see it there was no consensus reached on whether this
change should be made. I have some reservations as previously
outlined, but Alan seemed to be of the view that the current
situation was deliberately chosen - which implied to me (Alan
correct me if I'm wrong) that he opposed the change.
It may be that including this case in the OOM onError handling
is okay, but that the JVMTI event posting is not. But Alan will
need to clarify his position on that.
You got it. My view is that we should not post a JVM TI
ResourceExhausted event for this case.
I think Jeremy's original motive was to have the OnOutOfMemoryError
actions run. I don't see a problem changing the code to do that.
Yes, the current behavior is deliberate but this option is for
troubleshooting and maybe it can help with the (probably rare) cases
where this happens.
The other point I attempted to make is that if both
OnOutOfMemoryError and HeapDumpOnOutOfMemoryError are enabled then
we should always generate the heap dump before the
OnOutOfMemoryError run. I think we are in agreement that the heap
dump is not interesting here but we should still generate it anyway.
-Alan.
