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

Thu Jun 9 07:00:54 UTC 2011

Tony,

On 2011-06-09 04:09, Tony Printezis wrote:
>> 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.

No problem. I re-read my email and realized that I was being pretty unclear.

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

That's fine. Let's leave it as it is.

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

Ok. I see your point.

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

Thanks for elaborating a bit on this. I realize that this is a topic for 
a wider discussion than your review request, so we should maybe take 
this in another thread. Let me just say that I think there are several 
advantages to using "const" instead of "#define". As I mentioned it will 
be possible to see the values in debuggers, the compiler can do type 
checking and they respect scope.

Also, for all I know const in C++ is a compile time constant expression, 
so I don't know why a compiler would ever treat a const field as a 
non-constant. It is different in C of course.

Anyway, I think your review looks good. Ship it!
Bengt

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