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

[Tony Printezis]

Hi all,

Here's the updated webrev for this change:

http://cr.openjdk.java.net/~tonyp/7157073/webrev.1/webrev.all/

If you don't want to go through the whole thing again, here are the 
latest changes:

http://cr.openjdk.java.net/~tonyp/7157073/webrev.1/webrev.all/

Quick summary:

- The most important change was to use uintx for the "biased" region 
indexes. Because those indexes are wrt the bottom of the heap, and not 
the bottom of the table, they can get quite large. So it's safer to use 
uintx.

- A couple more size_t -> uint changes which (embarrassingly!) I seem to 
have missed first time round.

- We had some code in this style:

(size_t) (num * some_size)

or this style:

((size_t) num) * some_size

which, thanks to feedback from Igor, I standardized to this style:

(size_t) num * some_size

- In the end I gave in and I changed HeapRegionRemSet::init_heap() and 
::shrink_heap() to accept a uint instead of a size_t.

- We had several cases of (uint) 1 which I changed to 1U (thanks to 
feedback from Jon)

- It looks as if I don't need to change anything in the SA, the 
CIntegerField class is supposed to handle all the various integer types. 
I did update some comments in two SA classes though to reflect the 
updated types of some of the fields.

Something that I didn't do was to introduce types for indexes and nums. 
We discussed this during the G1 meeting today and we thought that it 
won't have a huge benefit in the long-term maintenance of the code.

The latest workspace compiles on all platforms and I'll run some extra 
testing overnight.

Let me know what you think.

Tony

On 03/27/2012 12:34 PM, 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
>