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

[Igor Veresov]

On a second thought, what is the rationale for doing this? This seems to be quite error prone I have to say. In case where you have to compute a size of something that is in units that are less than a region you have to take special care and do the cast. Like if you say: size_t region_offset_bytes = region_index << HeapRegion::LogOfHRGrainBytes; that would already be an error that is not easy to find - you'll just get a bogus answer and the compiler will be fine with it. 

Just my $0.02..

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/
> 
> 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 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120330/8b4e8193/attachment.htm>