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

[Tony Printezis]

Mikael and Igor,

Members of the GC group objected to the use of size_t for counts and 
indexes. Personally, I don't mind it given that it's less error-prone 
(IMHO) and I think a bit more consistent (less casting during heap size 
-> index calculations). But, I'm OK with the change too (if we have lots 
of counts / indexes in some data structures, they will be a little more 
cache friendly).

I'm happy if folks "fight it out" and tell me what to do - I'm OK either 
way.

Tony

On 03/30/2012 06:41 PM, Mikael Vidstedt wrote:
>
> 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
>>
>