Request for review (S): 7103665 HeapWord*ParallelScavengeHeap::failed_mem_allocate(unsigned long,bool)+0x97

[Bengt Rutisson]

Hi Vitaly,

On 2012-03-23 14:45, Vitaly Davidovich wrote:
>
> Hi Bengt,
>
> mutableNUMASpace line 99 has a small typo in the assert message: 
> "Remaining siz" should be "Remaining size".
>

Thanks for seeing it. Fixed.

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.

Thanks,
Bengt

> Vitaly
>
> Sent from my phone
>
> On Mar 23, 2012 9:06 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com 
> <mailto:bengt.rutisson at oracle.com>> wrote:
>
>
>     Hi all,
>
>     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.
>
>     http://cr.openjdk.java.net/~brutisso/7103665/webrev.00/
>     <http://cr.openjdk.java.net/%7Ebrutisso/7103665/webrev.00/>
>
>     Some background:
>
>     There are two problems that this fix solves:
>
>     (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.
>
>     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.
>
>     (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.
>
>     Here is the code that overflows:
>
>      328 void
>      329 CollectedHeap::fill_with_array(HeapWord* start, size_t words,
>     bool zap)
>      330 {
>      331   assert(words >= filler_array_min_size(), "too small for an
>     array");
>      332   assert(words <= filler_array_max_size(), "too big for a
>     single object");
>      333
>      334   const size_t payload_size = words - filler_array_hdr_size();
>      335   const size_t len = payload_size * HeapWordSize / sizeof(jint);
>      336
>      337   // Set the length first for concurrent GC.
>      338   ((arrayOop)start)->set_length((int)len);
>      339   post_allocation_setup_common(Universe::intArrayKlassObj(),
>     start, words);
>      340   DEBUG_ONLY(zap_filler_array(start, words, zap);)
>      341 }
>
>     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.
>
>     Testing:
>     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.
>
>     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.
>
>     Thanks,
>     Bengt
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120323/099bfaa8/attachment.htm>