<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#ffffff" text="#000000">
<br>
OK. Thanks for showing interest!<br>
<br>
Bengt<br>
<br>
On 2012-03-23 15:37, Vitaly Davidovich wrote:
<blockquote
cite="mid:CAHjP37EQWGm6CP=+_mrZVsS_g7KvOshGvZuXPfhxXpyM_2r3bQ@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt
0.8ex; border-left: 1px solid rgb(204, 204, 204);
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 moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com"
target="_blank">bengt.rutisson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin: 0pt 0pt
0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204);
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 moz-do-not-send="true"
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>
</blockquote>
<br>
</body>
</html>