RFR: 8134626 - Misc cleanups after generation array removal

Thomas Schatzl thomas.schatzl at oracle.com
Thu Sep 3 12:24:17 UTC 2015


Hi Jesper,

On Fri, 2015-08-28 at 16:37 +0200, Jesper Wilhelmsson wrote:
> Hi,
> 
> Please review this rather large cleanup. While working with the generation array 
> removal I did a bunch of unrelated cleanups in the code I passed by. I split 
> this out into a separate patch to keep the other changes clean.
> 
> This change is split into three webrevs to make it easier to review. I intend to 
> push all three as one change.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8134626
> 
> 
> Increment 1: Renaming
> http://cr.openjdk.java.net/~jwilhelm/8134626/webrev.00/inc1-renaming/
> 
> Changes the variable name gen to young_gen or old_gen depending on what the 
> variable will contain. The few cases where the name gen is untouched are places 
> where the variable actually can contain either generation.
> 

looks good.

> 
> Increment 2: Comments and indentation
> http://cr.openjdk.java.net/~jwilhelm/8134626/webrev.00/inc2-comments_and_indentation/
> 
> * Fixes up alignment in lots of places.
> * Inserts and removes empty lines where needed.
> * Cleans up comments where we previously talked about older and younger 
> generations. Fixes a few typos and clarifies some comments with respect to 
> young/old generations and collections.
> * Added spaces around some operators.
> 

concurrentMarkSweepGeneration.cpp, 2420, 3021, 5171, and 2492 (in the
new code): the parameter is actually called younger_gen_as_roots, so the
changes to young_gen_as_roots are wrong.

concurrentMarkSweepGeneration.cpp, the comment in 2946 seems useless,
because first it says "do X" and at the end "no, we actually don't do
X".

concurrentMarkSweepGeneration.inline.hpp 298: The sentence needs to be
put into singular form, i.e. "If the young gen collection has been
skipped, ..."

parNewGeneration.cpp, 539: upper case "if" needed

parNewGeneration.cpp, 983: "an" -> "and" (I think)

heapRegionType.hpp, line 38: I think the brackets after "major type"
should contain "young, old, humongous, archive" now

defNewGeneration.inline.hpp, 60: the comment should be updated to
reflect that we only have a single older generation now. I.e. "... p is
also in the old generation, but..."

Otherwise looks good. I do not need a re-review for this.

> 
> Increment 3: Code changes
> http://cr.openjdk.java.net/~jwilhelm/8134626/webrev.00/inc3-code_changes/
> 
> * Merged strings that was split on several lines for no good reason.
> * Added braces for if statements and for loops.
> * Removed dead code.
> * Moved variable initialization to initializer list in CollectedHeap constructor.
> * Updated flag descriptions "youngest" -> "young".

Looks good.

Thomas





More information about the hotspot-gc-dev mailing list