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

[Bengt Rutisson]

Hi Tony,

Thanks for doing this cleanup. Overall it looks good.

Some comments below. All of them are kind of picky. ;-)

Bengt


All casting back and forth between size_t and uint can probably be 
removed from:
CollectionSetChooser::calcMinOldCSetLength()
CollectionSetChooser::calcMaxOldCSetLength()

Could also be uint:
CollectionSetChooser::_lenght
CollectionSetChooser::_curr_index

That would make this case unnecessary:
uint remainingRegions() { return (uint) (_length - _curr_index); }


YoungList::reset_auxilary_lists()
Make young_index_in_cset an uint instead of casting it in the assert?


Could also take in uint:
HeapRegionRemSet::shrink_heap()
HeapRegionRemSet::init_heap()


This seems to be dead code. Remove instead of updating it?
G1CollectorPolicy::predict_young_collection_elapsed_time_ms()


g1CollectorPolicy.hpp
   static const uint REGIONS_UNLIMITED = ~(uint)0;
Not strictly related to your change, but I think a more common pattern 
in hotspot is:
    static const uint REGIONS_UNLIMITED = (uint)-1;
see for example:
  #define G1_NULL_HRS_INDEX ((uint) -1)


heapRegionRemSet.cpp

I think this:

void OtherRegionsTable::add_reference(OopOrNarrowOopStar from, int tid) {
   size_t cur_hrs_ind = (size_t) hr()->hrs_index();

could be:

void OtherRegionsTable::add_reference(OopOrNarrowOopStar from, int tid) {
   int cur_hrs_ind = hr()->hrs_index();

We use cur_hrs_ind as an index to OtherRegionsTable::_from_card_cache, 
which is an int**. BTW, that could be an uint** if thread ids were also 
uint, but that is probably not worth fixing with this change.


Several casts to size_t could instead be casts to BitMap::idx_t. Since 
we have that type I think we should probably use it. Some examples:

844 size_t max_hrs_index = (size_t) max->hr()->hrs_index();
895      if (!region_bm->at((size_t) cur->hr()->hrs_index())) {
1013  size_t hrs_ind = (size_t) from_hr->hrs_index();
1021  size_t hr_ind = (size_t) hr()->hrs_index();



Also, you seem to have introduced some tab characters. I guess jcheck 
will catch it, but here what I get when I grep for the tab character 
after applying your patch:

D:\repos\codereviews\hs-gc-tonyp-uint>grep -R -P -n '\t' 
src\share\vm\gc_implementation\g1
src\share\vm\gc_implementation\g1/concurrentMark.cpp:2408:                           
"appending %u entries to the secondary_free_list, "
src\share\vm\gc_implementation\g1/concurrentMark.cpp:2409:                           
"cleanup list still has %u entries",
src\share\vm\gc_implementation\g1/g1CollectedHeap.cpp:621:                                                      
size_t word_size) {
src\share\vm\gc_implementation\g1/g1CollectedHeap.cpp:3286:         
((size_t) young_regions) * HeapRegion::GrainBytes / K);
src\share\vm\gc_implementation\g1/g1CollectedHeap.cpp:3289:         
((size_t) survivor_regions) * HeapRegion::GrainBytes / K);
src\share\vm\gc_implementation\g1/g1CollectedHeap.hpp:461:                                       
size_t word_size);
src\share\vm\gc_implementation\g1/g1CollectedHeap.hpp:672:         
(size_t) (_in_cset_fast_test_length * sizeof(bool)));
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:435:                                 
(uint) (NewSize / HeapRegion::GrainBytes));
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:438:                             
(uint) (MaxNewSize / HeapRegion::GrainBytes));
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:446:                             
(uint) (MaxNewSize / HeapRegion::GrainBytes));
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:1807:                                       
uint survivor_cset_region_length) {
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:2351:                               
(uint) 1);
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:2353:                 
MinWorkUnit);
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:2359:                               
(uint) 1);
src\share\vm\gc_implementation\g1/g1CollectorPolicy.cpp:2361:                 
MinWorkUnit);
src\share\vm\gc_implementation\g1/g1CollectorPolicy.hpp:739:                                      
uint base_min_length,
src\share\vm\gc_implementation\g1/g1CollectorPolicy.hpp:740:                                      
uint desired_min_length,
src\share\vm\gc_implementation\g1/g1CollectorPolicy.hpp:741:                                      
uint desired_max_length);
src\share\vm\gc_implementation\g1/heapRegion.cpp:486:                  
G1BlockOffsetSharedArray* sharedOffsetArray,
src\share\vm\gc_implementation\g1/heapRegion.cpp:487:                  
MemRegion mr, bool is_zeroed) :
src\share\vm\gc_implementation\g1/heapRegionRemSet.cpp:876:                     
_n_coarse_entries);
src\share\vm\gc_implementation\g1/heapRegionRemSet.cpp:893:                            
cur->hr()->hrs_index());
src\share\vm\gc_implementation\g1/heapRegionRemSet.cpp:901:     }
src\share\vm\gc_implementation\g1/heapRegionRemSet.cpp:907:     }
src\share\vm\gc_implementation\g1/heapRegionRemSet.cpp:911:     }
src\share\vm\gc_implementation\g1/heapRegionSet.cpp:420:                     
name(), count, target_count));


On 2012-03-27 18:34, 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
>