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

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Tue Feb 10 20:38:26 UTC 2015


Hi Kim,

Thanks for taking another close look at this change!

Some of these issues are the result of this being a part of a bigger change, but 
I have addressed your concerns to make sure this intermediate version is of 
decent quality even without the following cleanups. (Even though I hope we will 
get the rest in asap. Those will be a lot easier to review).

For example anything related to levels and _n_gens will be removed in later 
patches when the entire level concept is removed.

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

Some replies inline.


Kim Barrett skrev den 9/2/15 21:05:
> 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.

I updated the assertion and removed the usage of nGens() in this method.

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

Assertion added and line removed.

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

Assertion added.

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

That may be true, even though I believe that the only major difference if this 
would have been done separately would be that the call sites would use 
get_gen(0/1) instead of young/old_gen. This is clearly the most difficult part 
of removing the generation array and also this is where we saw a few bugs in the 
last version. According to our tests it should be fine now. :)

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

I absolutely agree and I assume there are other simplifications and cleanups 
that will be apparent once the rest of the cleanups goes in as well. I copied 
your description above into an RFE to handle this issue.

https://bugs.openjdk.java.net/browse/JDK-8072809

>
> ------------------------------------------------------------------------------
> 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);

Agreed. Fixed.

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

Agreed. This was taken care of in one of the later cleanups but I moved that 
cleanup into this patch.

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

Assert fixed.

>
> ------------------------------------------------------------------------------
>

Thank you!
/Jesper


More information about the hotspot-gc-dev mailing list