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

[Bengt Rutisson]

Tony,

Thanks for iterating this so many times. I think it looks great now. 
Thumbs up!

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".

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