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

[Tony Printezis]

Igor,

Thanks for looking at it, I'll make the change.

Tony

On 03/27/2012 05:38 PM, Igor Veresov wrote:
> Looks good to me, except for one pedantic bit:
>
> In g1CollectedHeap.hpp:
>   671     memset(_in_cset_fast_test_base, false,
>   672            (size_t) (_in_cset_fast_test_length * sizeof(bool)));
>   673   }
> In case sizeof(bool)>  1, may be it should be:(size_t)_in_cset_fast_test_length * sizeof(bool)?
>
>
> igor
>
> On Tuesday, March 27, 2012 at 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/ 
>> <http://cr.openjdk.java.net/%7Etonyp/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/ 
>> <http://cr.openjdk.java.net/%7Etonyp/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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120402/60d6785c/attachment.htm>