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

[Bengt Rutisson]

Tony,

Thanks for addressing these comments so quickly! I am fine with this 
version. However, I realize that I was unclear with what I meant in my 
last email when I talked about reuse_if_possible(), now called 
expand_to_next_region().

What I meant was that I thought it would be a good idea to move all of 
this code into heapRegionSeq:

1632       HeapRegion* hr = _hrs.expand_to_next_region();
1633       if (hr == NULL) {
1634         // We'll have to create a new region.
1635         MemRegion mr(base, high);
1636         bool is_zeroed = !_g1_max_committed.contains(base);
1637         size_t hrs_index = _hrs.next_hrs_index();
1638         hr = new HeapRegion(hrs_index, _bot_shared, mr, is_zeroed);
1639         _hrs.expand_to_next_region(hr);
1640       } else {
1641         // We'll re-use the existing region.
1642         assert(hr->bottom() == base, "post-condition");
1643       }

I would wrap that in a method called something like get_next_heap_region().

I will not persist on this. And I do like the name expand_to_next() much 
better than the old name. I don't fully understand why you changed the 
name of HeapRegionSeq::insert(HeapRegion* hr) to 
HeapRegionSeq::expand_to_next_region(HeapRegion* hr) . I think "insert" 
was a better name. But maybe "append" would be more appropriate.

If the code above would be moved into heapRegionSeq we would not need to 
expose the insert/append/:expand_to_next_region(HeapRegion* hr) method.

Sorry for being difficult. But, as I said, I am fine with pushing it the 
way it is.

Also, just for my understanding, regarding "const" vs. "#define" you 
said "Maybe, I like the fact that they are guaranteed to be treated as 
constants by the C++ compilers". What do you mean by that?

Bengt


On 2011-06-08 18:57, Tony Printezis wrote:
> Bengt,
>
> Here's the new webrev that includes all the changes (I'll push them as 
> a single changeset):
>
> http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.0/webrev.all/ 
>
>
> And here are my latest the changes wrt to the version you reviewed:
>
> http://cr.openjdk.java.net/~tonyp/7045330.6804436.7042285/webrev.0/webrev.2.G1HRSLatest/ 
>
>
> Let me know if you think I haven't addressed your concerns.
>
> Tony
>
> On 06/08/2011 10:43 AM, Tony Printezis wrote:
>>
>>
>> On 06/08/2011 04:10 AM, Bengt Rutisson wrote:
>>>
>>> Tony,
>>>
>>> I had a look at this as well since it is connected to the previous 
>>> review: "CRR: 7042285 and 7045330 : fixes and improvements to the 
>>> HeapRegionSeq class (S/M) ".
>>>
>>> It looks good.
>>>
>>> Two things, though:
>>>
>>> * I think it is very closely connected to the previous review. So, 
>>> close that I in fact think they should be reviewed and pushed 
>>> together. I don't really see why they are separate changes.
>>
>> Yeah, you're right. I did a bunch of the int -> size_t changes as 
>> part of the first changeset (as I didn't think there was much point 
>> in rewriting half the HeapRegionSeq class using the wrong type and 
>> fixing it later). And, when I went it to do the rest it turned out 
>> there was not that many additional changes. When I push I'll fold 
>> both changesets into one and tag it with all three CRs.
>>
>>> * I still think it is a bit strange that G1_INVALID_HRS_INDEX is 
>>> defined in heapRegion.hpp. I would prefer to keep it in 
>>> heapRegionSeq. The only use in heapRegion is in the constructor. 
>>> This constructor is only called from two places:
>>>    - G1CollectedHeap::expand()
>>>       Here we actually know the index we want and could pass that to 
>>> the constructor. No need to first set it to an invalid value. I 
>>> think this would be even cleaner with the get_heap_region()  method 
>>> that I suggested in the previous review.
>>>   - G1CollectedHeap::initialize()
>>>        Here we know about heapRegionSeq and could ask it for the 
>>> invalid value and pass that to the constructor.
>>
>> Yeah, I was not crazy about having another "invalid" index. But maybe 
>> it's worth it. OK, I'll change that too and I'll post the new set of 
>> changes in a bit.
>>
>> Tony
>>
>>> Bengt
>>>
>>> On 2011-06-02 00:18, Tony Printezis wrote:
>>>> Stefan and John,
>>>>
>>>> Thanks! All set with this one too.
>>>>
>>>> Tony
>>>>
>>>> On 06/01/2011 03:11 PM, Stefan Karlsson wrote:
>>>>> Looks good.
>>>>>
>>>>> StefanK
>>>>>
>>>>> On 06/01/2011 02:04 PM, Tony Printezis wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Could I please have a couple of code reviews for this simple change:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~tonyp/6804436/webrev.0/
>>>>>>
>>>>>> Some of the int -> size_t changes were done as part of the 
>>>>>> HeapRegionSeq cleanup (see the separate code review request for 
>>>>>> 7042285 and 7045330 that I just sent out). This change does the 
>>>>>> rest (specifically: the changes related to the HeapRegion class).
>>>>>>
>>>>>> Note: I know that the webrev index says that some files have more 
>>>>>> than one change applied to them. This is incorrect and a 
>>>>>> side-effect of generating the webrev from a workspace with 
>>>>>> multiple patches stacked up. The diffs are actually correct (I 
>>>>>> checked).
>>>>>>
>>>>>> Tony
>>>>>>
>>>>>
>>>