<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jun 17, 2013 at 7:43 AM, Jon Masamitsu <span dir="ltr"><<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
On 6/17/13 2:25 AM, Thomas Schatzl wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
hi,<br>
<br>
On Thu, 2013-06-13 at 12:27 -0700, Hiroshi Yamauchi wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                 - instead of checking whether _eden_chunk_array is<br>
                 NULL to detect<br>
                 whether you can sample I would prefer if the same<br>
                 predicate as in the<br>
                 initialization (gch->supports_inline_contig_<u></u>alloc())<br>
                 were used.<br>
                 (I think the change just copied what was done in the<br>
                 previous version of<br>
                 concurrentMarkSweepGeneration.<u></u>cpp:4515 has been done)<br>
                                  Maybe create a new predicate for that, used<br>
                 everywhere.<br>
                                  (btw, throughout this code there is a lot of checking<br>
                 whether some<br>
                 NEW_C_HEAP_ARRAY returned NULL or not, and does<br>
                 elaborate recovery -<br>
                 however by default NEW_C_HEAP_ARRAY fails with an OOM<br>
                 anyway... - but<br>
                 that is certainly out of scope)<br>
                           Thomas, could you file a CR for fixing that?  Thanks.<br>
                  Thomas, by a new predicate, do you mean checking<br>
<br>
   if (gch->supports_inline_contig_<u></u>alloc())<br>
<br>
as opposed to<br>
<br>
   if (_eden_chunk_array != NULL)<br>
<br>
assuming that the NEW_C_HEAP_ARRAY succeeded?<br>
<br>
</blockquote>
If it did not succeed, the NEW_C_HEAP_ARRAY will exit the VM. This check<br>
is more because _eden_chunk_array is not initialized with a non-NULL<br>
pointer in all cases - exactly if supports_inline_contig_alloc() is not<br>
true, no space will be allocated for it.<br>
<br>
That's why such guards are needed.<br></blockquote></div></div></blockquote><div><br></div><div style>I see.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
You're right that the code checks the null-ness of _eden_chunk_array<br>
as CMSCollector::sample_eden() did so.<br>
<br>
I see that CR 8016276 was filed. Given this, but without knowing the<br>
</blockquote>
8016276 is about that there is no point in having allocation failure<br>
handling when the VM exits anyway on allocation failure.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  details of the CR, it seems OK for this patch to keep the null check<br>
against _eden_chunk_array as it is?<br>
<br>
</blockquote>
Yes, keep it. Also Jon seems to prefer it that way.<br>
</blockquote>
<br></div></div>
Yes.<br>
<br>
Thanks.<br>
<br>
Jon<br></blockquote><div><br></div><div style>OK.</div><div style> </div></div></div></div>