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

Thu May 7 14:48:36 UTC 2015

Hi Kim,

Thanks for reviewing and for giving valuable feedback!

A new webrev is available here:
http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.01/

And the increment:
http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.01.incremental/

More below.

Kim Barrett skrev den 20/4/15 21:28:
> 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?

Besides GenCollectedHeap there are two more classes that inherits CollectedHeap, 
G1CollectedHeap and ParallelScavengeHeap. Neither of these two uses the 
Generation class, so in order for gen to have a non-null value, heap must be a 
GenCollectedHeap. This is in no way obvious and the cast is, even though correct 
now, not extremely future proof, so I changed the implementation here. Let me 
know what you think.

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

There is code in PointerFinder.java that will set gen if it is a GenCollectedHeap.

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

My initial thought was that the unified logging work would fix this. I'm not 
sure if it matters too much if we make several changes or one big one with 
regard to external parsing tools that reads the output. The next patch in this 
series can clean up this logging if you think it would be better to do it sooner 
that the unified logging work.

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

Fixed.

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

Agreed. I rewrote ParNewRefProcTaskExecutor to take the old generation as a 
constructor argument as well. It cleaned up a few places somewhat.

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

Fixed.

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

Fixed.

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

Fixed.

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

It is now ;)  The next change contains random cleanups that has been found 
throughout this exercise. Some comments are updated there. I will take another 
look and try to find all references to "older" and "younger" generations.

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

No, I don't think so. It's on the list to be removed in a later change.

Thanks,
/Jesper