<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Igor,<br>
<br>
Thanks for looking at it, I'll make the change.<br>
<br>
Tony<br>
<br>
On 03/27/2012 05:38 PM, Igor Veresov wrote:
<blockquote cite="mid:D98431E8F76B4DD99A15E335AB9AD961@oracle.com"
type="cite">
<div>Looks good to me, except for one pedantic bit: </div>
<div><br>
</div>
<div> In g1CollectedHeap.hpp:</div>
<div>
<pre style="font-family: Times; font-size: 13px; "> 671 memset(_in_cset_fast_test_base, false,
<a moz-do-not-send="true" name="12"></a><span style="color: blue; "> 672 (size_t) (_in_cset_fast_test_length * sizeof(bool)));</span>
673 }</pre>
<pre><font face="Helvetica">In case sizeof(bool) > 1, may be it should be: </font><span style="color: rgb(0, 0, 255); font-family: Times; font-size: 13px; ">(size_t)_in_cset_fast_test_length * sizeof(bool) </span><span style="font-family: Helvetica; ">?</span></pre>
</div>
<div>
<div><br>
</div>
<div><br>
</div>
<div>igor</div>
<div><br>
</div>
</div>
<p style="color: #A0A0A8;">On Tuesday, March 27, 2012 at 9:34 AM,
Tony Printezis wrote:</p>
<blockquote type="cite">
<div> <span>
<div>
<div>
<div>Hi all,</div>
<div><br>
</div>
<div>I'd like a couple of reviews for this cleanup. We
have been using </div>
<div>size_t's to represent region counts / indexes and I
have been encouraged </div>
<div>to change those to uint's. This unfortunately
turned out to be quite an </div>
<div>extensive change that was hard to localize. I've
split the change into </div>
<div>two webrevs.</div>
<div><br>
</div>
<div>First, this breaks down a long line in
g1CollectedHeap.cpp. I I have it </div>
<div>on a separate webrev as it messes up the webrev
pages for the rest of </div>
<div>the changes (i.e., the rest of the changes are
easier to read with this </div>
<div>excluded):</div>
<div><br>
</div>
<div><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etonyp/7157073/webrev.0/webrev.0.G1LongLineFix/">http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.0.G1LongLineFix/</a></div>
<div><br>
</div>
<div>The main body of the changes is here:</div>
<div><br>
</div>
<div><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etonyp/7157073/webrev.0/webrev.1.G1Uints/">http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.1.G1Uints/</a></div>
<div><br>
</div>
<div>Some important comments:</div>
<div><br>
</div>
<div>* I tried to change as many of the size_t
occurrences as possible but I </div>
<div>didn't touch the following:</div>
<div><br>
</div>
<div>- HeapRegionRemSet - we're in the process of
reworking this code so I </div>
<div>decided to leave it mostly as is (I only added a
few necessary casts)</div>
<div><br>
</div>
<div>- CollectionSetChooser / HeapRegion::sort_index() /
</div>
<div>HeapRegion::_sort_index - I have a CR open for a
CSetChooser cleanup and </div>
<div>I'll do the conversion as part of that cleanup (I
did change a few types </div>
<div>on the CSetChooser external API to avoid casts from
methods that access it).</div>
<div><br>
</div>
<div>- HeapRegion::_young_index_in_cset /
HeapRegion::young_index_in_cset() - </div>
<div>The code looks as if it relies on the fact that
this is an int. It seems </div>
<div>well localized so I left it as is and we can look
at it on a separate CR.</div>
<div><br>
</div>
<div>* I had to make some changes to the
vmStructs_g1.hpp and I need to </div>
<div>ensure that the SA code works with those changes.
Unfortunately, I'm </div>
<div>going on vacation for a few days tonight and I
haven't had a chance to </div>
<div>do that yet. I'll look at the SA more closely when
I get back and, if I </div>
<div>need to make any changes to the SA, I'll publish
those separately (they </div>
<div>will be self-contained anyway). I just wanted to
get the main body of </div>
<div>the changes out before I leave so any potential
reviewers can get </div>
<div>started on it.</div>
<div><br>
</div>
<div>* Testing: the source builds on all platforms and
passes JPRT. And I did </div>
<div>a lot of extra 64-bit testing (as any issues should
mainly show up in </div>
<div>64-bit builds).</div>
<div><br>
</div>
<div>Tony</div>
</div>
</div>
</span> </div>
</blockquote>
<div> <br>
</div>
</blockquote>
</body>
</html>