RFR: JDK-8077842 - Remove the level parameter passed around in GenCollectedHeap

[Kim Barrett]

On Apr 15, 2015, at 1:59 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Hi,
> 
> Please review the next cleanup in the generation cleanup series. This time the 'level' is at stake. We pass it around in lots of places but in reality we already know which level (i.e. generation) we want to work with, so the level is just noise. In some places we want to verify that we are in the correct generation, or we pass a generation on to some generic code that can work on both young and old, in these cases we are now more explicit and makes it clear which generation we are dealing with instead of just sending a number.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8077842
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.00/
> 
> Thanks,
> /Jesper

------------------------------------------------------------------------------
agent/src/share/classes/sun/jvm/hotspot/utilities/PointerLocation.java
  87     return ((gen != null) && (gen == ((GenCollectedHeap)heap).getGen(0)));
and
  91     return ((gen != null) && (gen == ((GenCollectedHeap)heap).getGen(1)));

Strictly speaking, it seems like the GenCollectedHeap cast ought to be
protected by a try/catch for a bad cast.  Else why is there a
CollectedHeap base class at all?

On the other hand, maybe I'm missing something, but I don't see any
code to assign a value to the "gen" field.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
 688     // I didn't want to change the logging when removing the level concept,
 689     // but I guess this logging could say "old" or something instead of "1".
 690     assert(this == gch->old_gen(),
 691            "The CMS generation should be the old generation");
 692     uint level = 1;

I'm assuming this comment and the retention of level is still
intended, for the reason give.  Is there a followup cleanup RFE for
this?

Similarly in genCollectedHeap.cpp, line 332 and following.
Similarly in generation.cpp, line 115 and following.

------------------------------------------------------------------------------
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
5650   // If the young generation has been collected. Gather any statistics

Comment needs work.  Maybe the period after "collected" should be a
comma?

------------------------------------------------------------------------------ 
src/share/vm/gc_implementation/parNew/parNewGeneration.cpp
 827   ParNewRefProcTaskProxy rp_task(task, _generation, *(gch->old_gen()),

Getting the old_gen like this at this point seems strange.  We were
given the (young) generation at construction time, as a constructor
argument, but here we're going off to the gch to get the old_gen.

------------------------------------------------------------------------------ 
src/share/vm/memory/defNewGeneration.cpp 
  60 DefNewGeneration::IsAliveClosure::IsAliveClosure(Generation* g) : _g(g) { }

This seems to be expecting g to be the young_gen.  There used to be an
assertion, but that assertion has been removed.  I think the assertion
should be retained.

------------------------------------------------------------------------------
src/share/vm/memory/defNewGeneration.cpp 
 106 void DefNewGeneration::FastEvacuateFollowersClosure::do_void() {
 107   do {
 108     _gch->oop_since_save_marks_iterate(Generation::Young, _scan_cur_or_nonheap,
 109                                        _scan_older);
 110   } while (!_gch->no_allocs_since_save_marks(Generation::Young));
 111   guarantee(_gen->promo_failure_scan_is_complete(), "Failed to finish scan");
 112 }

The class is now hard-coded to be for the young generation.  If the
_gen in the final guarantee were changed to obtain the young_gen from
the heap then the generation constructor argument and member variable
would no longer be needed, and could no longer accidentally be wrong.

------------------------------------------------------------------------------ 
src/share/vm/memory/defNewGeneration.cpp 
 370   // This is called after a gc that includes the following generation
 371   // (which is required to exist.)  So from-space will normally be empty.

This comment about "the following generation" needs updating.

------------------------------------------------------------------------------
src/share/vm/memory/genCollectedHeap.hpp
 376   // Invoke the "do_oop" method of one of the closures "not_older_gens"
 377   // or "older_gens" on root locations for the generations depending on
 378   // the type.  (The "older_gens" closure is used for scanning references
 379   // from older generations; "not_older_gens" is used everywhere else.)

Is cleanup of the terminology in this comment and the associated
parameter names intended to be part of a later part of this series?
"older_gens" no longer makes sense, for example; it should just be
"old_gen_cl" or something like that.

------------------------------------------------------------------------------ 
src/share/vm/memory/genCollectedHeap.cpp
1121 GCStats* GenCollectedHeap::gc_stats(Generation* gen) const {
1122   return gen->gc_stats();

Is GenCollectedHeap::gc_stats() worth keeping now?

------------------------------------------------------------------------------