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

Tue Apr 3 16:11:43 UTC 2012

So, Bengt made his case. Any objections to this getting pushed?

Tony

On 04/02/2012 10:09 AM, Bengt Rutisson wrote:
>
> 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
>>>>>
>>>>
>