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

[Tony Printezis]

Bengt,

On 06/10/2011 09:23 AM, Bengt Rutisson wrote:
>
> Tony,
>
>>> 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()).

Apologies for the bad grammar! It of course meant to say: "...that has 
the FreeRegionList..."

> I see. I tried chaning ...Sets.hpp to ..Seq.hpp and it compiled, so I 
> assumed that this was what you needed. But you are of course right 
> about FreeRegionList. The ...Sets.hpp must be included through one of 
> the other header files since I got it to compile. We should of course 
> include it explicitly as you did.
>
> On the other hand I think we should also explicitly include the "own" 
> header file. So, it would be good to add ...Seq.hpp as well.

The heapRegionSeq header file is being included from 
g1CollectedHeap.hpp. But, sure, I'll add it explicitly (in fact I think 
I'll need the .inline.hpp version).

Tony

> Thanks,
> Bengt
>
>>
>> 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
>>>
>