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

Thu Apr 12 14:18:00 UTC 2012

Hi Jesper,

Inline.

On 04/12/2012 05:26 AM, Jesper Wilhelmsson wrote:
> Tony,
>
> In general it looks good, 

Thanks.

> but there are a couple of cases where I don't understand why you 
> choose to leave the size_t and add casts instead of just use uint all 
> over the place.
>
> The function below is one example of this:
>
>  276 uint CollectionSetChooser::calcMinOldCSetLength() {
>  277   // The min old CSet region bound is based on the maximum desired
>  278   // number of mixed GCs after a cycle. I.e., even if some old 
> regions
>  279   // look expensive, we should add them to the CSet anyway to make
>  280   // sure we go through the available old regions in no more than 
> the
>  281   // maximum desired number of mixed GCs.
>  282   //
>  283   // The calculation is based on the number of marked regions we 
> added
>  284   // to the CSet chooser in the first place, not how many remain, so
>  285   // that the result is the same during all mixed GCs that follow 
> a cycle.
>  286
>  287   const size_t region_num = (size_t) _length;
>  288   const size_t gc_num = (size_t) G1MixedGCCountTarget;
>  289   size_t result = region_num / gc_num;
>  290   // emulate ceiling
>  291   if (result * gc_num < region_num) {
>  292     result += 1;
>  293   }
>  294   return (uint) result;
>  295 }
>
> Why are region_num, gc_num and result size_t in this code?
>
> I'm not asking you to change anything, I just want to understand why 
> this choice was made so that we don't mess things up by changing it to 
> uint later.

I'm doing a separate cleanup on the CSet Chooser class, which includes 
more changes like some renaming, dead code removal, etc. So, for this 
CR, I only decided to make enough changes to the external API of the 
CSet chooser class so that the callers use uint's and I'll cleanup the 
implementation on the other CR (it's done BTW, I'm currently merging it 
with this change).

Hope this explains things?

Tony

> /Jesper
>
>
> 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
>>>