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

Bengt Rutisson bengt.rutisson at oracle.com
Tue Dec 2 09:29:42 UTC 2014


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/

Thanks,
Bengt

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




More information about the hotspot-gc-dev mailing list