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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Dec 9 13:20:21 UTC 2014


Hi Bengt,

  changes look good to me (sans Kim's suggestions) - following there are
a few nitpicks about some minor code style issues that could be fixed.
Feel free to ignore ones that cause too much effort.

On Mon, 2014-12-08 at 16:20 +0100, Bengt Rutisson wrote:
> 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.
> >>>

Not sure if you want to fix up some coding style issues too:

- braces in cardGeneration.cpp:54, 
- capital initial letter for the comment of
CardGeneration::_shrink_factor

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

- capital initial letter of comment for TenuredGeneration::_the_space

Feel free to ignore.

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

(I saw that comments in concurrentMarkSweepGeneration.hpp start with
lower case letter. I think it's best to ignore this as these are a lot).

- maybe the lines that only contain two forward slashes could be
removed, in concurrentMarkSweepGeneration.hpp:175,177

- Some copyright dates were not updated (I am not insisting on them, but
apparently there has been put some effort in doing that, so it would be
nice to do all of them), in particular in the .inline.hpp files.

- maybe the empty lines in tenuredGeneration.cpp:260, 265, 285 could be
removed.

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list