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

Mon Apr 2 14:09:50 UTC 2012

Hi everyone,

I like this change because I think it adds information. I know I'm 
probably wrong about this in a strict meaning (based on the definition 
of size_t) but here's how I see it:

To me size_t communicates that this is a variable that contains a value 
that is somehow related to a memory size. For example, I want to 
allocate an object of a size_t size or a region has a size_t size.

If I want to have a value that is related to the memory size, but is not 
a size but rather based on a pointer I would use intptr_t. Even though 
the compiler might allow me to do similar operations on a size_t and on 
most (all our supported) platforms they will have the same width. But it 
adds information to say intptr_t instead of size_t. It tells the reader 
that this is actually a pointer and not a size.

In the same fashion I think something that is a "count" - number of 
something - should be some kind of int-value rather than a size_t. That 
communicates to the reader that this is a number and not a size or a 
pointer.

Personally I think "number of regions" in G1 is just such a case. It is 
not a memory size, so for me it adds extra information to change it from 
size_t to int. For this particular value I also think it shows the 
intent better as well. We are aiming for 5000 regions or something in 
that order. We are not seriously suggesting billions of regions. And if 
we were, I think we should use the int64_t type to communicate that.

Not sure if this made any sense. I won't insist on this being pushed -  
it just makes much more sense to me.

Bengt

On 2012-04-02 15:29, Tony Printezis wrote:
> PS Peter Kessler suggested the use of specific types for region counts 
> and indexes, like:
>
> typedef uint region_count_t;
> typedef uint region_index_t;
>
> to make a little more explicit what variables / fields represent. I 
> think it's a good idea. If we decide to go with this change, let me 
> know if this is something I should also add.
>
> Tony
>
> On 04/02/2012 09:27 AM, Tony Printezis wrote:
>> 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
>>>>
>>>