RFR: 8055702 - Remove the generations array

Thu Oct 16 21:39:42 UTC 2014

On Oct 14, 2014, at 9:28 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Hi all,
> 
> Resurrecting this thread from the past.
> I got one review from Mikael, need another to clean this up.
> 
> Thanks!
> /Jesper
> 
> 
> Jesper Wilhelmsson skrev 22/8/14 15:20:
>> […]
>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8055702/webrev/
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8055702

==============================================================================
agent/src/share/classes/sun/jvm/hotspot/memory/GenCollectedHeap.java
 77     if ((i < 0) || (i >= nGens())) {
 78       return null;
 79     }
 80 
 81     if (i == 0) {
 82       return genFactory.newObject(youngGenField.getAddress());
 83     } else {
 84       return genFactory.newObject(oldGenField.getAddress());
 85     }

Most of the java code seems to still be agnostic about the number of
generations.  This is the one place where the limit of 2 generations
is being hard-wired.  I think it would be appropriate to protect this
code with some sort of check that nGens() < 2.

Optionally, I think it would now be simpler to merge the two if
statements into a single switch statement, e.g.

   switch (i) {
   case 0:
       return genFactory.newObject(youngGenfield.getAddress());
   case 1:
       return genFactory.newObject(oldGenfield.getAddress());
   default:
       // no generation for i, and assertions disabled.
       return null;
   }

[Of course, leave the line 72-75 assertion in place.]

============================================================================== 
src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
957         prev_size = prev_gen->capacity();
958           gclog_or_tty->print_cr("  Younger gen size "SIZE_FORMAT,
959                                  prev_size/1000);

The logging statement at 958 appears to be mis-indented.
[Pre-existing problem.]

============================================================================== 
src/share/vm/memory/genCollectedHeap.cpp
141   ReservedSpace young_rs = heap_rs.first_part(_gen_specs[0]->max_size(), false, false);
142   _young_gen = _gen_specs[0]->init(young_rs, 0, rem_set());
143   heap_rs = heap_rs.last_part(_gen_specs[0]->max_size());
144 
145   ReservedSpace old_rs = heap_rs.first_part(_gen_specs[1]->max_size(), false, false);
146   _old_gen = _gen_specs[1]->init(old_rs, 1, rem_set());
147   heap_rs = heap_rs.last_part(_gen_specs[1]->max_size());

Could we have a helper function for this near duplicated code?

There's a bunch of hard-coded 0s and 1s here for young and old
respectively.

This also makes me wonder if _gen_specs[] should also be eliminated;
it looks like it was a parallel array to the _gens[] array, with the
latter being no longer present.

234 void GenCollectedHeap::save_used_regions(int level) {
235   assert(level < _n_gens, "Illegal level parameter");
236   if (level == 1) {
237     _old_gen->save_used_region();
238   }
239   _young_gen->save_used_region();
240 }

More hard-coded 0s (implicit) and 1s here for young and old
respectively.

I looked at the changes to do_collection() involved in factoring out
new collect_generation().  I *think* they are ok, but it's hard to be
sure because the webrev diffs are a bit of a mess.  It might be easier
to review if collect_generation() followed do_collection() rather than
preceeding it.  [Of course, making that change would make a mess of an
incremental webrev.]

============================================================================== 
src/share/vm/memory/genCollectedHeap.hpp
373   Generation* prev_gen(Generation* gen) const {
374     int l = gen->level();
375     guarantee(l == 1, "Out of bounds");
376     return _young_gen;
377   }
378 
379   // Return the generation after "gen".
380   Generation* next_gen(Generation* gen) const {
381     int l = gen->level() + 1;
382     guarantee(l == 1, "Out of bounds");
383     return _old_gen;
384   }
385 
386   Generation* get_gen(int i) const {
387     guarantee(i >= 0 && i < _n_gens, "Out of bounds");
388     if (i == 0) return _young_gen;
389     else return _old_gen;
390   }

Suggest changing next_gen() to

 Generation* next_gen(Generation* gen) const {
   int l = gen->level();
   guarantee(l == 0, "Out of bounds");
   return _old_gen;
 }

to eliminate the +1 and be more consistent with prev_gen().

Oh, but maybe this doesn't matter.  I just re-read the webrev request
and see that get_gen(), next_gen(), and prev_gen() are slated to go
away. 

Again here, many hard-coded 0s and 1s for young and old respectively.

BTW, "l" (lowercase L) and "1" (integer one) look *very* similarly in
some fonts, making "l" (lowercase L) a poor choice of variable name.
I had to look really closely at the webrev to see that "l == 1" was
not tautological.  Similar issue with "O" (capital o) and 0 (integer
zero).

============================================================================== 
src/share/vm/memory/genMarkSweep.cpp
161   // Scratch request on behalf of oldest generation; will do no
162   // allocation.
163   ScratchBlock* scratch = gch->gather_scratch(gch->get_gen(gch->_n_gens-1), 0);

Why not "gch->old_gen()" instead of "gch->get_gen(gch->_n_gens-1)" ?

==============================================================================  
src/share/vm/memory/generation.cpp
162 Generation* Generation::next_gen() const {
163   GenCollectedHeap* gch = GenCollectedHeap::heap();
164   int next = level() + 1;
165   if (next < gch->_n_gens) {
166     return gch->get_gen(next);
167   } else {
168     return NULL;
169   }
170 }

Perhaps instead

Generation* Generation::next_gen() const {
 GenCollectedHeap* gch = GenCollectedHeap::heap();
 if (this == gch->young_gen()) {
   return gch->old_gen();
 } else {
   assert(this == gch->old_gen());
   return NULL;
 }
}

But again here, maybe this next_gen() is also slated to go away?

==============================================================================
src/share/vm/runtime/vmStructs.cpp
547     static_field(GenCollectedHeap,             _gch,                                          GenCollectedHeap*)                     \
548  nonstatic_field(GenCollectedHeap,             _n_gens,                                       int)                                   \

These two lines appear to be incorrectly indented wrto the surrounding
parts of this file.

============================================================================== 