<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 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 href="http://cr.openjdk.java.net/~tonyp/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 href="http://cr.openjdk.java.net/~tonyp/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>