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