RFR (M): 8066780, 8066781, 8066782: Cleanup of duplicated code in TenuredGeneration and ConcurrentMarkSweepGeneration

[Bengt Rutisson]

On 2014-12-08 10:32, Bengt Rutisson wrote:
>
> Hi Kim,
>
> Thanks for looking at this!
>
> On 2014-12-05 20:07, Kim Barrett wrote:
>> On Dec 5, 2014, at 10:18 AM, Bengt Rutisson 
>> <bengt.rutisson at oracle.com> wrote:
>>> I was looking at the duplicated code in TenuredGeneration and 
>>> ConcurrentMarkSweepGeneration and wanted to clean it up. I decided 
>>> to split the cleanup into three parts (thanks Kim for suggesting 
>>> that in a pre-review). The parts are closely related so I think it 
>>> is a good idea to review them together. Thus, I'm asking for reviews 
>>> for all three changes in this same email.
>>>
>>> Here are the three changes:
>>>
>>> Split CardGeneration out to its own file
>>> https://bugs.openjdk.java.net/browse/JDK-8066780
>>> http://cr.openjdk.java.net/~brutisso/8066780/webrev.00/
>>>
>>> Just moving code from generations.cpp/hpp to cardGeneration.cpp/hpp.
>>>
>>>
>>> Minor cleanups to TenuredGeneration
>>> https://bugs.openjdk.java.net/browse/JDK-8066781
>>> http://cr.openjdk.java.net/~brutisso/8066781/webrev.00/
>>>
>>> Removing some dead code and making sure that the include guard in 
>>> the inline file is correctly named. The _last_gc variable was 
>>> exported to the SA agent but the SA agent never referenced it.
>>>
>>> Move common code from CMSGeneration and TenuredGeneration to 
>>> CardGeneration
>>> https://bugs.openjdk.java.net/browse/JDK-8066782
>>> http://cr.openjdk.java.net/~brutisso/8066782/webrev.00/
>>>
>>> This is the actual move of common code from TenuredGeneration and 
>>> ConcurrentMarkSweepGeneration to CardGeneration. Both subclasses use 
>>> a single Space instance and by allowing CardGeneration to find that 
>>> space instance it was possible to move many of the methods that work 
>>> on the Space instance up to the CardGeneration too.
>>>
>>> One of the changes is to rename the expand() method in 
>>> ConcurrentMarkSweepGeneration that took three paramteres to 
>>> expand_for_gc_reason(). I did this to avoid that anyone thinks that 
>>> ConcurrentMarkSweepGeneration overrides the CardGeneration::expand() 
>>> method. This change is included in the webrev above, but if it makes 
>>> things easier I also split it out to a separate webrev:
>>>
>>> http://cr.openjdk.java.net/~brutisso/8066782/expand_for_gc_cause.00/
>> Looks good.  Just a couple of formatting comments below.
>>
>> Thanks for splitting it up into several webrevs; that hugely
>> simplified the review.  [The last one is still a bit tricky, but much
>> easier when isolated from the rest.]
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>> src/share/vm/memory/tenuredGeneration.inline.hpp
>>    44   if (addr < _the_space->top()) return oop(addr)->size();
>>    45   else {
>>    46     assert(addr == _the_space->top(), "non-block head arg to 
>> block_size");
>>    47     return _the_space->end() - _the_space->top();
>>    48   }
>>
>> I'm not sure I've ever before seen that formatting style for an "if",
>> and I'm finding it painful to parse.  I also think it violates the
>> Hotspot StyleGuide, e.g.
>>    Use braces around substatements. (Relaxable for extremely simple
>>    substatements on the same line.)
>> Would you mind adding braces, and a line break before the first
>> return, since you are touching these lines anyway?
>
> Quite agree. Horrible formatting! I changed it to:
>
> size_t TenuredGeneration::block_size(const HeapWord* addr) const {
>   if (addr < _the_space->top()) {
>     return oop(addr)->size();
>   }
>   else {
>     assert(addr == _the_space->top(), "non-block head arg to 
> block_size");
>     return _the_space->end() - _the_space->top();
>   }
> }

A small update. Stefan J. just pointed out that the else statement is a 
on a separate line. I prefer to have it on the same line as the "}".  
So, I've updated my change to use:


size_t TenuredGeneration::block_size(const HeapWord* addr) const {
   if (addr < _the_space->top()) {
     return oop(addr)->size();
   } else {
     assert(addr == _the_space->top(), "non-block head arg to block_size");
     return _the_space->end() - _the_space->top();
   }
}

Bengt

>
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>> src/share/vm/memory/cardGeneration.cpp
>> 187
>> 194
>>
>> A couple of stray blank lines inserted.
>
> Thanks for noting this. I removed these blank lines.
>
> Bengt
>
>
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>