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

[John Coomes]

Tony Printezis (tony.printezis at oracle.com) wrote:
> PS Peter Kessler suggested the use of specific types for region counts 
> and indexes, like:
> 
> typedef uint region_count_t;
> typedef uint region_index_t;
> 
> to make a little more explicit what variables / fields represent. I 
> think it's a good idea. If we decide to go with this change, let me know 
> if this is something I should also add.

Sorry for chiming in late, I was on vacation.

I think giving names to widely used types like these is important, so
the next time the representation is changed :-), we don't have the
same pain.

-John

> On 04/02/2012 09:27 AM, Tony Printezis wrote:
> > Mikael and Igor,
> >
> > Members of the GC group objected to the use of size_t for counts and 
> > indexes. Personally, I don't mind it given that it's less error-prone 
> > (IMHO) and I think a bit more consistent (less casting during heap 
> > size -> index calculations). But, I'm OK with the change too (if we 
> > have lots of counts / indexes in some data structures, they will be a 
> > little more cache friendly).
> >
> > I'm happy if folks "fight it out" and tell me what to do - I'm OK 
> > either way.
> >
> > Tony
> >
> > On 03/30/2012 06:41 PM, Mikael Vidstedt wrote:
> >>
> >> I'm also interested in the background to the change. For structures 
> >> that are very common in memory it makes sense to streamline the size 
> >> of the fields and structs. Is that the case here or is something else 
> >> driving the change?
> >>
> >> Thanks,
> >> Mikael
> >>
> >> On 2012-03-30 15:06, Igor Veresov wrote:
> >>> 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/ 
> >>>> <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
> >>>
> >>