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

Thu Apr 12 14:34:12 UTC 2012

12 apr 2012 kl. 16:18 skrev Tony Printezis <tony.printezis at oracle.com>:

> Hi Jesper,
> 
> Inline.
> 
> On 04/12/2012 05:26 AM, Jesper Wilhelmsson wrote:
>> Tony,
>> 
>> In general it looks good, 
> 
> Thanks.
> 
>> 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.
> 
> I'm doing a separate cleanup on the CSet Chooser class, which includes more changes like some renaming, dead code removal, etc. So, for this CR, I only decided to make enough changes to the external API of the CSet chooser class so that the callers use uint's and I'll cleanup the implementation on the other CR (it's done BTW, I'm currently merging it with this change).
> 
> Hope this explains things?
> 

Absolutely, thanks!
Ship it!
/Jesper

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