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

[Bengt Rutisson]

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()).

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.

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