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

Thu Jun 9 02:09:31 UTC 2011

Bengt,

On 06/08/2011 05:00 PM, Bengt Rutisson wrote:
>
> 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:

Ah, I see. Apologies I missed that point.

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

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.

> And I do like the name expand_to_next() much better than the old name. 

Good!

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

Well, both expand_to_next_region() and insert() essentially do the same 
thing: they expand the sequence by one region, either by re-using an 
existing one or by adding a new one. So, I thought having a similar name 
was 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?

In case the compiler decides not to the treat the const field as a 
constant for whatever reason...

Tony

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