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

[Bengt Rutisson]

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();
   }
}

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


>
> ------------------------------------------------------------------------------
>