RFR (M): JDK-8065993: Merge OneContigSpaceCardGeneration with TenuredGeneration

[Mikael Gerdin]

On 2014-12-02 10:29, Bengt Rutisson wrote:
>
> Hi Kim,
>
> Thanks for looking at this!
>
>
> On 2014-12-01 18:53, Kim Barrett wrote:
>> [resending with correct from address, so it will go to list]
>>
>> On Dec 1, 2014, at 7:42 AM, Bengt Rutisson <bengt.rutisson at oracle.com>
>> wrote:
>>> Could I have a couple of reviews to this cleanup?
>>>
>>> http://cr.openjdk.java.net/~brutisso/8065993/webrev.00/
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8065993
>> ------------------------------------------------------------------------------
>>
>>
>> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>>
>> 2815 // YSR: All of this generation expansion/shrinking stuff is an
>> exact copy of
>> 2816 // TenuredGeneration, which makes me wonder if we should move this
>> 2817 // to CardGeneration and share it...
>>
>> [Comment was updated to refer to TenuredGeneration.]
>>
>> Perhaps there should be a bug report for this cleanup?  I did a brief
>> search and didn't find one.
>
> Right. I started looking at this to file a bug for it. But (as expected)
> the two implementations have diverged and I need to do some more
> investigation to find out what can be done now. I'll file a bug when I
> have more information. We should either move the code to CardGeneration
> or remove the comment.
>
>>
>> ------------------------------------------------------------------------------
>>
>>
>> src/share/vm/memory/generation.hpp
>>   53 // The system configurations currently allowed are:
>>   54 //
>>   55 //   DefNewGeneration + TenuredGeneration
>>   56 //   DefNewGeneration + ConcurrentMarkSweepGeneration
>>   57 //
>>   58 //   ParNewGeneration + TenuredGeneration
>>   59 //   ParNewGeneration + ConcurrentMarkSweepGeneration
>>
>> This pre-existing comment near a comment that was changed looks a
>> little stale.  Wasn't DefNew + CMS recently removed?
>
> Good catch. Removed the two lines that are no longer correct.
>
>>
>> ------------------------------------------------------------------------------
>>
>>
>> src/share/vm/memory/space.hpp
>>
>> Why ContiguousSpace declaring friend with OneContigSpaceCardGeneration
>> replaced by Space declaring friend with TenuredGeneration?  That
>> movement is surprising, and seems like it might indicate a bug
>> somewhere.
>
> I moved the friend declaration from ContigousSpace to Space because the
> reason for the friend declaration was that
> TenuredGeneration::block_size() wanted to access the protected instance
> variable Space::_end. It seemed more correct to friend the class
> declaring it than some random subclass. But looking at it again I
> realized that there is actually a public getter that can be used
> instead. So, thanks for catching this. By using the getter I could
> completely get rid of the ugly friend declaration.
>
> New webrev:
> http://cr.openjdk.java.net/~brutisso/8065993/webrev.01/
>
> Diff compared to last one:
> http://cr.openjdk.java.net/~brutisso/8065993/webrev.00-01.diff/

Still looks good to me.
/Mikael

>
> Thanks,
> Bengt
>
>>
>> ------------------------------------------------------------------------------
>>
>>
>> Only the last item, about the friendship change, is directly relevant
>> to the proposed changes, which otherwise look good.
>>
>