CRR (M): 7157073: G1: type change size_t -> uint for region counts / indexes

[Bengt Rutisson]

Looks good.

Bengt

On 2012-04-12 00:08, Tony Printezis wrote:
> Hi all,
>
> Here's the updated webrev for this change:
>
> http://cr.openjdk.java.net/~tonyp/7157073/webrev.1/webrev.all/
>
> If you don't want to go through the whole thing again, here are the 
> latest changes:
>
> http://cr.openjdk.java.net/~tonyp/7157073/webrev.1/webrev.all/
>
> Quick summary:
>
> - The most important change was to use uintx for the "biased" region 
> indexes. Because those indexes are wrt the bottom of the heap, and not 
> the bottom of the table, they can get quite large. So it's safer to 
> use uintx.
>
> - A couple more size_t -> uint changes which (embarrassingly!) I seem 
> to have missed first time round.
>
> - We had some code in this style:
>
> (size_t) (num * some_size)
>
> or this style:
>
> ((size_t) num) * some_size
>
> which, thanks to feedback from Igor, I standardized to this style:
>
> (size_t) num * some_size
>
> - In the end I gave in and I changed HeapRegionRemSet::init_heap() and 
> ::shrink_heap() to accept a uint instead of a size_t.
>
> - We had several cases of (uint) 1 which I changed to 1U (thanks to 
> feedback from Jon)
>
> - It looks as if I don't need to change anything in the SA, the 
> CIntegerField class is supposed to handle all the various integer 
> types. I did update some comments in two SA classes though to reflect 
> the updated types of some of the fields.
>
> Something that I didn't do was to introduce types for indexes and 
> nums. We discussed this during the G1 meeting today and we thought 
> that it won't have a huge benefit in the long-term maintenance of the 
> code.
>
> The latest workspace compiles on all platforms and I'll run some extra 
> testing overnight.
>
> Let me know what you think.
>
> Tony
>
> On 03/27/2012 12:34 PM, Tony Printezis wrote:
>> Hi all,
>>
>> I'd like a couple of reviews for this cleanup. We have been using 
>> size_t's to represent region counts / indexes and I have been 
>> encouraged to change those to uint's. This unfortunately turned out 
>> to be quite an extensive change that was hard to localize. I've split 
>> the change into two webrevs.
>>
>> First, this breaks down a long line in g1CollectedHeap.cpp. I I have 
>> it on a separate webrev as it messes up the webrev pages for the rest 
>> of the changes (i.e., the rest of the changes are easier to read with 
>> this excluded):
>>
>> http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.0.G1LongLineFix/ 
>>
>>
>> The main body of the changes is here:
>>
>> http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.1.G1Uints/
>>
>> Some important comments:
>>
>> * I tried to change as many of the size_t occurrences as possible but 
>> I didn't touch the following:
>>
>> - HeapRegionRemSet - we're in the process of reworking this code so I 
>> decided to leave it mostly as is (I only added a few necessary casts)
>>
>> - CollectionSetChooser / HeapRegion::sort_index() / 
>> HeapRegion::_sort_index - I have a CR open for a CSetChooser cleanup 
>> and I'll do the conversion as part of that cleanup (I did change a 
>> few types on the CSetChooser external API to avoid casts from methods 
>> that access it).
>>
>> - HeapRegion::_young_index_in_cset / 
>> HeapRegion::young_index_in_cset() - The code looks as if it relies on 
>> the fact that this is an int. It seems well localized so I left it as 
>> is and we can look at it on a separate CR.
>>
>> * I had to make some changes to the vmStructs_g1.hpp and I need to 
>> ensure that the SA code works with those changes. Unfortunately, I'm 
>> going on vacation for a few days tonight and I haven't had a chance 
>> to do that yet. I'll look at the SA more closely when I get back and, 
>> if I need to make any changes to the SA, I'll publish those 
>> separately (they will be self-contained anyway). I just wanted to get 
>> the main body of the changes out before I leave so any potential 
>> reviewers can get started on it.
>>
>> * Testing: the source builds on all platforms and passes JPRT. And I 
>> did a lot of extra 64-bit testing (as any issues should mainly show 
>> up in 64-bit builds).
>>
>> Tony
>>