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

Tue Apr 3 16:13:09 UTC 2012

Jon,

No need to apologize, I queued the e-mail while travelling yesterday and 
sent it when I got home. Clearly, it arrived in your inbox with an 
earlier timestamp.

Tony

On 04/02/2012 10:14 PM, Jon Masamitsu wrote:
> 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
>>>>