<div>
On a second thought, what is the rationale for doing this? This seems to be quite error prone I have to say. In case where you have to compute a size of something that is in units that are less than a region you have to take special care and do the cast. Like if you say: size_t region_offset_bytes = region_index << HeapRegion::LogOfHRGrainBytes; that would already be an error that is not easy to find - you'll just get a bogus answer and the compiler will be fine with it.
</div>
<div><div><br></div><div>Just my $0.02..</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" style="border-left-style:solid;border-width:1px;margin-left:0px;padding-left:10px;">
<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>
</blockquote>
<div>
<br>
</div>