RFR: JDK-8061802 - REDO - Remove the generations array

[Kim Barrett]

On Jan 29, 2015, at 5:45 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Hi,
> 
> Please review this second attempt to remove the generations array.
> 
> There were two bugs that caused this patch to be backed out the last time:
> 
> 1. Collection of the old generation was always run even if the young collection freed up enough to satisfy the allocation need. This was due to an unexpected use of the size variable and stopped working when the code that changed the variable was broken out into a separate function.
> 
> 2. The new _young_generation and _old_generation fields was missing from the declarations in vm_structs.cpp. Cut'n'paste error when the original huge change was split into smaller parts for easier review.
> 
> I have resolved these issues. I also moved the BiasedLocking::preserve_marks() since the previous change didn't preserve exactly the same behavior. And I added a comment in a test that caused some issues when I was debugging this.
> 
> Testing: AdHoc run of the nightly GC tests, JPRT, and JTREG
> 
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8061802
> 
> Webrev: http://cr.openjdk.java.net/~jwilhelm/8061802/webrev.01/
> 
> I also made an incremental webrev based on the old patch applied to a fresh version of the GC repo. Please note that the old patch do not apply cleanly to a current GC repo, but the changes in the incremental diff should cover what has been changed with regards to this RFE. Please refer to the full webrevs if in doubt.
> 
> Incremental webrev:
> http://cr.openjdk.java.net/~jwilhelm/8061802/webrev.01.incremental/
> 
> Old (buggy) webrev: http://cr.openjdk.java.net/~jwilhelm/8055702/webrev.01/
> 
> 
> For reference, as I mentioned above the original huge change was split into several smaller parts. This is the first of those parts. The other can be found here:
> 
> http://cr.openjdk.java.net/~jwilhelm/8057632/webrev.01/
> 
> Please note that these are the old patches that applies on top of the old (buggy) patch above. They will not apply cleanly on top of the new patch. I'll update these once the first part is finalized.
> 
> Thanks,
> /Jesper

------------------------------------------------------------------------------  
Throughout, copyrights need to be updated.

------------------------------------------------------------------------------  
agent/src/share/classes/sun/jvm/hotspot/memory/GenCollectedHeap.java
  71   public Generation getGen(int i) {
  72     if (Assert.ASSERTS_ENABLED) {
  73       Assert.that((i >= 0) && (i < nGens()), "Index " + i +
  74                   " out of range (should be between 0 and " + nGens() + ")");
  75     }
  76 
  77     switch (i) {
  78     case 0:
  79       return genFactory.newObject(youngGenField.getAddress());
  80     case 1:
  81       return genFactory.newObject(oldGenField.getAddress());
  82     default:
  83       // no generation for i, and assertions disabled.
  84       return null;
  85     }
  86   }

With the replacement of the old code with the switch statement, the
assertion seems both poorly placed and not entirely sufficient.  The
code is now assuming that nGens() == 2, for example.

------------------------------------------------------------------------------  
src/share/vm/memory/genCollectedHeap.cpp
 131   ReservedSpace young_rs = heap_rs.first_part(_gen_specs[0]->max_size(), false, false);
 132   _young_gen = _gen_specs[0]->init(young_rs, 0, rem_set());
 133   heap_rs = heap_rs.last_part(_gen_specs[0]->max_size());
 134 
 135   ReservedSpace old_rs = heap_rs.first_part(_gen_specs[1]->max_size(), false, false);
 136   _old_gen = _gen_specs[1]->init(old_rs, 1, rem_set());
 137   heap_rs = heap_rs.last_part(_gen_specs[1]->max_size());

I feel like there should be an assertion somewhere before here that
_n_gens == 2.

Also, I think line 137 is unnecessary.

------------------------------------------------------------------------------  
src/share/vm/memory/genCollectedHeap.cpp 
 216 void GenCollectedHeap::save_used_regions(int level) {
 217   assert(level < _n_gens, "Illegal level parameter");
 218   if (level == 1) {
 219     _old_gen->save_used_region();
 220   }
 221   _young_gen->save_used_region();
 222 }

Assumes level >= 0.  Old code would do nothing with negative level.
Probably negative level is a bug, and should be asserted against.

------------------------------------------------------------------------------  
src/share/vm/memory/genCollectedHeap.cpp 
collect_generation and do_collection

I tried to give the changes here a thorough look, but the diffs are
very messy.  I hope I didn't miss anything.

I think the review of this would have been noticably easier if the
extraction of collect_generation into a separate function had happened
before and independently of the generation array changes.  Oh well.

------------------------------------------------------------------------------  
src/share/vm/memory/genCollectedHeap.cpp 
 574 void GenCollectedHeap::
 575 gen_process_roots(int level,

I think the proposed changes maintain the previous behavior, as
desired.  However ...

This is probably something for a future (but soon, I hope) cleanup.

GenCollectedHeap::gen_process_roots() is a bit of a mess now. It first
calls SharedHeap::process_roots. It then does completely different
things, depending on the value of the first (level) argument. That's
not at all obvious from the way the code is written though. Retaining
the structure from the old iteration through _gens[] is not helpful in
the new world where we have exactly two generations.

(I was surprised to note that the old code seems to do nothing for
_gens[level]. I wonder if that was a lurking bug?)

There are two overloads for gen_process_roots().  The first does most
of the work.  The second constructs an additional closure and calls
the first with that closure plus some other argument adjustments.  I
think all client callers call the second; the first could be hoisted
into the second and eliminated as a separate function.

I also suspect that, with some careful analysis, we could determine
the level value for all callers. This and the fact that this function
does very different things according to the level suggests splitting
this function into two distinct functions.

Here's what I came up with for the level values being provided:

- MarkSweep always calls with level == 1 (assertion)
- CMS always calls with level == _cmsGen->level()
  presumably _cmsGen->level() is 1, since _cmsGen is old_gen?
- ParNewGeneration::work calls with level == _gen->level()
  _gen is the young generation, so level is always 0 here?
- DefNewGeneration::collect calls with level == _level
  _level is 0, since this is young generation?

The two callers apparently providing 0 as the level also always
provide true as the second (younger_gens_as_roots) argument.  I
suspect there are other argument simplifications that would result
from splitting.

I'm ok with the proposed change as is, as part of that change set.
But please make sure there's a follow-on cleanup RFE.

------------------------------------------------------------------------------ 
src/share/vm/memory/genCollectedHeap.cpp 
 837   // This might be sped up with a cache of the last generation that
 838   // answered yes.
 839   if (_young_gen->is_in(p) || _old_gen->is_in(p)) {
 840     return true;
 841   }
 842   // Otherwise...
 843   return false;

The comment on line 837 seems mostly relevant to the old code that
iterated through the generations array.  Is it still relevant?
Especially since it seems to bottom out in Space::is_in(), which is
documented as being potentially slow and so only to be used in assert.

Also, with only the two generations now, I'd probably write this as

  return _young_gen->is_in(p) || _old_gen->is_in(p);

------------------------------------------------------------------------------ 
src/share/vm/memory/genCollectedHeap.cpp 
1046 void GenCollectedHeap::prepare_for_compaction() {
1047   guarantee(_n_gens = 2, "Wrong number of generations");
1048   Generation* old_gen = _old_gen;
1049   // Start by compacting into same gen.
1050   CompactPoint cp(old_gen);
1051   old_gen->prepare_for_compaction(&cp);
1052   Generation* young_gen = _young_gen;
1053   young_gen->prepare_for_compaction(&cp);
1054 }

The old_gen and young_gen local variables just add clutter after the
rewrite.

------------------------------------------------------------------------------ 
src/share/vm/memory/genCollectedHeap.hpp
 387   Generation* get_gen(int i) const {
 388     guarantee(i >= 0 && i < _n_gens, "Out of bounds");
 389     if (i == 0) {
 390       return _young_gen;
 391     } else {
 392       return _old_gen;
 393     }
 394   }

The assertion here now ought to be (level == 0 || level == 1).  And
_n_gens must be 2.

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