CRR (M): 7157073: G1: type change size_t -> uint for region counts / indexes
Tony Printezis
tony.printezis at oracle.com
Mon Apr 2 15:16:16 UTC 2012
Bengt,
Thanks for looking at it. Inline,
On 03/30/2012 04:29 AM, Bengt Rutisson wrote:
>
> Hi Tony,
>
> Thanks for doing this cleanup. Overall it looks good.
>
> Some comments below. All of them are kind of picky. ;-)
No worries, picky is my middle name. ;-)
> 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); }
As I said in my CRR e-mail:
"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)."
So I'll do all the above as part of that CR if that's OK....
> YoungList::reset_auxilary_lists()
> Make young_index_in_cset an uint instead of casting it in the assert?
I avoided changing anything related to the young_index_in_cset (and it's
not only used in the assert, it's also passed to the
set_region_survivor_method()):
"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'll leave it as is and open a CR for this.
> Could also take in uint:
> HeapRegionRemSet::shrink_heap()
> HeapRegionRemSet::init_heap()
"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)"
> This seems to be dead code. Remove instead of updating it?
> G1CollectorPolicy::predict_young_collection_elapsed_time_ms()
(well spotted) This is indeed removed as part of the conc mark cleanup
changes and I didn't want to remove it in both changesets to keep the
merge between them (maybe) easier. I only added the cast to make sure
that it compiles stand-alone. But, yes, it will go away. So, don't worry
about it too much.
> 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)
Agreed. Fixed.
static const uint REGIONS_UNLIMITED = (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();
As I said, I'll leave the RSet files as is, as they should be replaced
soon-ish.
> Also, you seem to have introduced some tab characters.
(re-installed Linux, forgot to configure emacs not to use tabs....)
Damn, jcheck did NOT catch this (I'm pretty sure I ran it on the
changeset). I'll fix those. Thanks for spotting this.
Tony
> 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
>>
>
More information about the hotspot-gc-dev
mailing list