RFR: 8055702 - Remove the generations array

Fri Oct 17 12:25:07 UTC 2014

Hi Kim!

Thanks for looking at this change!

This cleanup is followed by five more found in the review thread "RFR: 8057632 - 
Remove auxiliary code used to handle the generations array"

I chose to split this rather large cleanup into several pieces to make it easier 
to review it. So, if you are OK with a few intermediate versions of less beauty, 
I would prefer to read your comments in the light of the last version after all 
cleanups have been made.

A new webrev is available here that fixes the java code mentioned below:
http://cr.openjdk.java.net/~jwilhelm/8055702/webrev.01/

See comments inline.

Kim Barrett skrev 16/10/14 23:39:
> 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.]
>

I like the switch so I'm using it as suggested.

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

This is fixed in one of the later cleanups.

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

I wrote the code in this way deliberately to make it easy to review, 
_gen_specs[] is going away in a later cleanup, so are all 0s and 1s. The final 
version of this code looks as follows:

   ReservedSpace young_rs = 
heap_rs.first_part(gen_policy()->young_gen_spec()->max_size(), false, false);
   _young_gen = gen_policy()->young_gen_spec()->init(young_rs, rem_set());
   heap_rs = heap_rs.last_part(gen_policy()->young_gen_spec()->max_size());

   ReservedSpace old_rs = 
heap_rs.first_part(gen_policy()->old_gen_spec()->max_size(), false, false);
   _old_gen = gen_policy()->old_gen_spec()->init(old_rs, rem_set());
   heap_rs = heap_rs.last_part(gen_policy()->old_gen_spec()->max_size());

There are a lot of similarities between the young and old handling here but 
personally I don't see a benefit in breaking this code out into a helper function.

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

All hard coded 0s and 1s are going away in one of the following cleanups.

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

Do you have access to a slightly more advanced diff tool like Beyond Compare for 
instance? (Free demo version available online.)  With that tool you can choose 
which lines to align and diff this part more easily.

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

Yes, these three are going away in one of the later cleanups.

> ==============================================================================
> 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)" ?
>

In this change I tried to remove the generations array without changing too much 
other code in order to make it easier to review it. All cleanups enabled by this 
change has been performed in later changes (I hope). In this particular case 
old_gen() is used in the final version as suggested.

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

Yes, next_gen() is also removed in one of the later cleanups.

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

_n_gens is going away, but _gch stays and was actually still one space off. I 
fixed that now in the last cleanup change. I'll upload a new webrev and ping the 
other mail thread about that.

> ==============================================================================
>

Thanks!
/Jesper