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

[Jon Masamitsu]

Sorry about the late response.  I didn't see this when I looked earlier.

Responses in-line

On 4/2/2012 8:03 AM, Tony Printezis wrote:
> Hi Jon,
>
> Thanks for looking at this. Inline.
>
> On 03/28/2012 12:11 AM, Jon Masamitsu wrote:
>> Tony,
>>
>> Looks good.  A few questions.
>>
>> http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.1.G1Uints/src/share/vm/gc_implementation/g1/concurrentMark.cpp.frames.html 
>>
>>
>>> 1841     BitMap::idx_t index = (BitMap::idx_t) hr->hrs_index();
>>> 1842     if (!hr->startsHumongous()) {
>>> 1843       // Normal (non-humongous) case: just set the bit.
>>> 1844       _region_bm->par_set_bit(index);
>>> 1845     } else {
>>> 1846       // Starts humongous case: calculate how many regions are 
>>> part of
>>> 1847       // this humongous region and then set the bit range.
>>> 1848       G1CollectedHeap* g1h = G1CollectedHeap::heap();
>>> 1849       HeapRegion *last_hr = 
>>> g1h->heap_region_containing_raw(hr->end() - 1);
>>> 1850       size_t end_index = (BitMap::idx_t) last_hr->hrs_index() + 1;
>>> 1851       _region_bm->par_at_put_range(index, end_index, true);
>>
>> Why the choice to make index (line 1841) a BitMap::idx_t and 
>> end_index (line 1850) a size_t?  Seems like
>> end_index could also be BitMap::idx_t.
>
> Thanks for spotting this, it looks as if I just forgot to change the 
> type of end_index (there was a similar change earlier on where I 
> seemed to have done the right thing). Fixed.

Thanks.

>
>>> 1709           gclog_or_tty->print_cr("Region %u: card bitmap 
>>> mismatch at " SIZE_FORMAT ": "
>>> 1710                                  "expected: %d, actual: %d",
>>> 1711                                  hr->hrs_index(), i, expected, 
>>> actual);
>>
>> Since you changed the format for hr->hrs_index() to %u, maybe change 
>> the other format
>> to $u also?
>
> Which "other" format?
>
> i is of type BitMap::idx_t (which is basically size_t), so SIZE_FORMAT 
> is appropriate for it
> expected and actual are bool's

Sorry about the fuzzy comment.  The different types of indexes confused me.
You changed the format for one type of index to %u and I thought the other
type of indexes would also be %u but they are different types of indexes so
it's fine as is.  What you have below is fine also.

>
> Does the following work better?
>
>           gclog_or_tty->print_cr("Region %u: card bitmap mismatch at " 
> SIZE_FORMAT ": "
>                                  "expected: %s, actual: %s",
>                                  hr->hrs_index(), i,
>                                  BOOL_TO_STR(expected), 
> BOOL_TO_STR(actual));
>
> and I also changed an earlier similar message to the following too:
>
>         gclog_or_tty->print_cr("Region %u: region bitmap mismatch: "
>                                "expected: %s, actual: %s",
>                                hr->hrs_index(),
>                                BOOL_TO_STR(expected), 
> BOOL_TO_STR(actual));
>
>
>> http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.1.G1Uints/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp.frames.html 
>>
>>
>>> 2042   const uint max_region_idx = ((uint)1<<  
>>> (sizeof(RegionIdx_t)*BitsPerByte-1)) - 1;
>> You might have been able to use 1U instead of (uint) 1.
>
> I *think* I've seen it both ways in the code and I don't have a strong 
> preference. I'll go with 1U.
Thanks.  Yes, I've seen it both ways too.  1U seems a little nicer.


>
>>> 2078   HeapRegionRemSet::init_heap((size_t) max_regions());
>>
>> I would have changed the parameter to init_heap() to uint.  Only one 
>> call site.
>
> init_heap() will go away when we replace the RSet code, so I'll leave 
> it as is for the moment.

OK.

>
>> http://cr.openjdk.java.net/~tonyp/7157073/webrev.0/webrev.1.G1Uints/src/share/vm/gc_implementation/g1/sparsePRT.cpp.udiff.html 
>>
>>
>>> -  gclog_or_tty->print_cr("  Adding card %d from region %d to region "
>>> -                         SIZE_FORMAT" sparse.",
>>> +  gclog_or_tty->print_cr("  Adding card %d from region %d to region 
>>> %u sparse.",
>>>                            card_index, region_id, _hr->hrs_index());
>>
>> Why stay with the %d format?  Same thing here
>>
>>
>>> -  gclog_or_tty->print_cr("  Expanded sparse table for "SIZE_FORMAT" 
>>> to %d.",
>>> +  gclog_or_tty->print_cr("  Expanded sparse table for %u to %d.",
>>>                           _hr->hrs_index(), _next->capacity());
>
> SparsePRT is part of the RSet structures and will go away when the 
> RSets are replaced (in fact, that whole file should go away). So, I 
> didn't want to spend too much time on that... I'll leave it as is if 
> that's OK.

Ok.

Jon
>
> Tony
>
>> That's all.
>>
>> Jon
>>
>> On 3/27/2012 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/ 
>>>
>>>
>>> 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
>>>