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

[Jesper Wilhelmsson]

Tony,

In general it looks good, but there are a couple of cases where I don't 
understand why you choose to leave the size_t and add casts instead of just use 
uint all over the place.

The function below is one example of this:

  276 uint CollectionSetChooser::calcMinOldCSetLength() {
  277   // The min old CSet region bound is based on the maximum desired
  278   // number of mixed GCs after a cycle. I.e., even if some old regions
  279   // look expensive, we should add them to the CSet anyway to make
  280   // sure we go through the available old regions in no more than the
  281   // maximum desired number of mixed GCs.
  282   //
  283   // The calculation is based on the number of marked regions we added
  284   // to the CSet chooser in the first place, not how many remain, so
  285   // that the result is the same during all mixed GCs that follow a cycle.
  286
  287   const size_t region_num = (size_t) _length;
  288   const size_t gc_num = (size_t) G1MixedGCCountTarget;
  289   size_t result = region_num / gc_num;
  290   // emulate ceiling
  291   if (result * gc_num < region_num) {
  292     result += 1;
  293   }
  294   return (uint) result;
  295 }

Why are region_num, gc_num and result size_t in this code?

I'm not asking you to change anything, I just want to understand why this 
choice was made so that we don't mess things up by changing it to uint later.
/Jesper


On 2012-04-12 00:08, Tony Printezis wrote:
> 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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: jesper_wilhelmsson.vcf
Type: text/x-vcard
Size: 251 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20120412/afb6883a/jesper_wilhelmsson.vcf>