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

[Tony Printezis]

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.

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

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.

>> 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.

> 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.

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