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

[Jon Masamitsu]

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.

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

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.


> 2078   HeapRegionRemSet::init_heap((size_t) max_regions());

I would have changed the parameter to init_heap() to uint.  Only one 
call site.


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());


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
>