CRR (M): 7157073: G1: type change size_t -> uint for region counts / indexes
Tony Printezis
tony.printezis at oracle.com
Tue Apr 3 13:11:40 UTC 2012
Bengt,
On 04/03/2012 03:23 AM, Bengt Rutisson wrote:
>
> Tony,
>
> Sorry for not reading your original email careful enough. I clearly
> missed that you intentionally avoided some changes. Thanks for
> reminding me.
No problem. I could have carried on cleaning those up too, but I just
had to stop somewhere. Otherwise, the changeset would just get too large.
> I'm fine with your comments below.
>
> Ship it!
Thanks! BTW, also thanks for pointing out that I had tab chars in the
changeset. jcheck did find them so clearly I had used it wrongly somehow
(ran it on the wrong directory maybe?). Anyway, I've removed them now...
Tony
> 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