<p>I'm not a reviewer, just a curious bystander. :)</p>
<p>Sent from my phone</p>
<div class="gmail_quote">On Mar 23, 2012 10:32 AM, "Bengt Rutisson" <<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    <br>
    Hi Vitaly,<br>
    <br>
    On 2012-03-23 14:45, Vitaly Davidovich wrote:
    <blockquote type="cite">
      <p>Hi Bengt,</p>
      <p>mutableNUMASpace line 99 has a small typo in the assert
        message: "Remaining siz" should be "Remaining size".</p>
    </blockquote>
    <br>
    Thanks for seeing it. Fixed.<br>
    <br>
    BTW, you don't have an OpenJDK username, right? If you consider this
    a review I can list you as "Also-reviewed-by", when I push.<br>
    <br>
    Thanks,<br>
    Bengt<br>
    <br>
    <blockquote type="cite">
      <p>Vitaly</p>
      <p>Sent from my phone</p>
      <div class="gmail_quote">On Mar 23, 2012 9:06 AM, "Bengt Rutisson"
        <<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>>
        wrote:<br type="attribution">
        <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
          <br>
          Hi all,<br>
          <br>
          Could I have a couple of reviews for this change? This is on
          the critical-watch list for hs23, so it would be great if I
          could get some reviews already today.<br>
          <br>
          <a href="http://cr.openjdk.java.net/%7Ebrutisso/7103665/webrev.00/" target="_blank">http://cr.openjdk.java.net/~brutisso/7103665/webrev.00/</a><br>
          <br>
          Some background:<br>
          <br>
          There are two problems that this fix solves:<br>
          <br>
          (1) When we run with -XX:+UseNUMA we have to make the heap
          parsable since we leave some holes in the heap. We do this by
          faking Integer arrays in those places. However, on a large
          heap those holes can be larger than what can be covered by a
          single Integer array. The fix is to allocate several arrays in
          those cases.<br>
          <br>
          The easy fix would have been to call CollectedHeap::fill_with_objects()
          instead of CollectedHeap::fill_with_object(). Note the extra
          "s" in the method name. But MutableNUMASpace::ensure_parsability(),
          where the change is needed, need to know about all the objects
          that are allocated so I saw no better solution than to
          implement my own loop. Any ideas on how I could re-use
          fill_with_objects() instead are welcome.<br>
          <br>
          (2) CollectedHeap::_filler_array_max_size should be the
          maximum size that can be covered by a fake Integer array. This
          value is in words, but since the word size is different on
          different platforms but the Integer size is always the same we
          need to convert between word and sizeof(jint) at some point.
          Unfortunately we do the conversion in the wrong direction,
          which means that on 64 bit platforms we end up with a max size
          that is 4 times larger than intended. This in turn makes us
          overflow an int when we convert from a word size to the length
          of the array that we are setting up.<br>
          <br>
          Here is the code that overflows:<br>
          <br>
           328 void<br>
           329 CollectedHeap::fill_with_array(HeapWord* start, size_t
          words, bool zap)<br>
           330 {<br>
           331   assert(words >= filler_array_min_size(), "too small
          for an array");<br>
           332   assert(words <= filler_array_max_size(), "too big
          for a single object");<br>
           333<br>
           334   const size_t payload_size = words -
          filler_array_hdr_size();<br>
           335   const size_t len = payload_size * HeapWordSize /
          sizeof(jint);<br>
           336<br>
           337   // Set the length first for concurrent GC.<br>
           338   ((arrayOop)start)->set_length((int)len);<br>
           339   post_allocation_setup_common(Universe::intArrayKlassObj(),
          start, words);<br>
           340   DEBUG_ONLY(zap_filler_array(start, words, zap);)<br>
           341 }<br>
          <br>
          If filler_array_max_size() on line 332, which returns
          CollectedHeap::_filler_array_max_size, has a too large value
          we will overflow the int conversation on line 338.<br>
          <br>
          Testing:<br>
          This fix solves the issue that was found in the reproducer
          that I could set up on a Linux x64 machine. I have asked the
          one who initially reported the issue to run on their Solaris
          amd64 setup. I will also try to set up a reproducer on a
          Solaris machine.<br>
          <br>
          I also ran the fix through JPRT. It did not fail, but there
          are no NUMA tests in there as far as I know. But the change
          for issue (2) was hopefully tested.<br>
          <br>
          Thanks,<br>
          Bengt<br>
        </blockquote>
      </div>
    </blockquote>
    <br>
  </div>

</blockquote></div>