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

[Mikael Vidstedt]

I'm also interested in the background to the change. For structures that 
are very common in memory it makes sense to streamline the size of the 
fields and structs. Is that the case here or is something else driving 
the change?

Thanks,
Mikael

On 2012-03-30 15:06, Igor Veresov wrote:
> 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.
>
> Just my $0.02..
>
> igor
>
> On Tuesday, March 27, 2012 at 9:34 AM, 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/ 
>> <http://cr.openjdk.java.net/%7Etonyp/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/ 
>> <http://cr.openjdk.java.net/%7Etonyp/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
>