CRR (M): 7157073: G1: type change size_t -> uint for region counts / indexes
Bengt Rutisson
bengt.rutisson at oracle.com
Tue Apr 3 07:23:45 UTC 2012
Tony,
Sorry for not reading your original email careful enough. I clearly
missed that you intentionally avoided some changes. Thanks for reminding me.
I'm fine with your comments below.
Ship it!
Bengt
On 2012-04-02 17:16, Tony Printezis wrote:
> 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