CRR: 6804436: G1: heap region indices should be size_t (S)

[Tony Printezis]

On 06/10/2011 02:30 AM, Bengt Rutisson wrote:
>
> Tony,
>
> Thanks for iterating this so many times. I think it looks great now. 
> Thumbs up!

Thanks for looking at all the iterations! I ran tests on it last night 
and everything is OK. I'll schedule it for a push today.

> Just one nit:
>
> In heapRegionSeq.cpp you added the include 
> "gc_implementation/g1/heapRegionSets.hpp". I think that the one you 
> want is "gc_implementation/g1/heapRegionSeq.hpp".

Actually, ...Sets.hpp is correct as it's the one that FreeRegionList 
declaration (needed by expand_by()).

Tony

> Bengt
>
> On 2011-06-10 00:46, Tony Printezis wrote:
>> Bengt,
>>
>> Here's the latest version:
>>
>> complete webrev:
>> http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.1/webrev.all/ 
>>
>>
>> diffs from the previous one:
>> http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.1/webrev.1.G1HRSFactory/ 
>>
>>
>> I created a factory method for HeapRegions, G1CH::new_heap_region(), 
>> combined the two expand_to_next_region() methods of HeapRegionSeq 
>> class into one (HRS::expand_by()), and updated G1CH::expand() 
>> accordingly.
>>
>> I did notice however that the HeapRegion constructor could fail to 
>> allocate space, if we're out of swap. Even though it's a marginal 
>> condition, JohnC2 recently put some effort to make the expansion code 
>> robust wrt to expansion failing. So, I made some additional changes 
>> to deal with the case where we cannot allocate more HeapRegions (the 
>> committed space will be shrunk accordingly).
>>
>> An additional minor change is that I renamed the HTS fields _end and 
>> _bottom to _heap_end and _heap_bottom to make what they represent a 
>> bit clearer.
>>
>> Better?
>>
>> Tony
>>
>>
>> On 06/09/2011 07:11 AM, Tony Printezis wrote:
>>> Bengt,
>>>
>>> On 06/09/2011 03:00 AM, Bengt Rutisson wrote:
>>>>>>
>>>>>> I will not persist on this.
>>>>>
>>>>> This is a good suggestion, Bengt, but I'll need to expose more 
>>>>> things from G1CH to the HeapRegionSeq to do that (i.e., the fields 
>>>>> needed in the HeapRegion constructor). And I just didn't want to 
>>>>> re-arrange things further. So, I think I'll leave it as is, i.e., 
>>>>> G1CH creating the regions and passing them on to HeapRegionSeq.
>>>>
>>>>
>>>> That's fine. Let's leave it as it is.
>>>
>>> Actually (and I should have thought of that earlier!) I think I can 
>>> do this by creating a factory method for HeapRegion instances in 
>>> G1CH and calling it from HRS. So HRS will only have a single 
>>> expand() method (opposite to shrink()) and will deal with the rest 
>>> internally. Let me have a stab at it and I'll get back to you.
>>>
>>> Tony
>