RFR: JDK-8061802 - REDO - Remove the generations array
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Tue Feb 17 21:08:43 UTC 2015
Hi Mikael,
Thanks for reviewing!
I agree with your comments and has fixed them in this new webrev:
cr.openjdk.java.net/~jwilhelm/8061802/webrev.03
Thanks,
/Jesper
Mikael Gerdin skrev den 13/2/15 13:58:
> Hi Jesper,
>
> On 2015-02-10 21:38, Jesper Wilhelmsson wrote:
>> 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/
>
> Overall I think this is a good incremental cleanup.
>
> I have some requests to make it easier to reason about the code though:
>
> if (!(max_level == 1 && full && _old_gen->full_collects_younger_generations()) &&
> _young_gen->should_collect(full, size, is_tlab))
>
> is a bit of a mouthful, can you please break out parts of it into a boolean?
>
> bool old_collects_young = (max_level == 1 && full &&
> _old_gen->full_collects_younger_generations())
>
> if (old_collects_young && _young_gen->should_collect...)
>
>
> The must_restore_marks_for_biased_locking is even more confusing.
>
> Its initial value is "false", DefNew returns false for
> performs_in_place_marking() so the if block there is essentially dead code.
>
> For the old generation you pass !must_restore_marks.. to collect_generation in
> order to trigger the preserving of marks if it was not performed completed by
> the young gc but it's then immediately set to true after the call to
> collect_generation in order to get the code to properly restore the marks.
>
> I suggest that you assert that young gens always return false for
> performs_in_place_marking() and either pass literal "true" or "false" to
> collect_generation, or perhaps even call BiasedLocking::preserve_marks() before
> calling collect_generation for old gen and just get rid of the parameter.
>
> You would still need to keep the boolean variable in order to know if you should
> restore the marks, but that's fine I think.
>
> /Mikael
>
>
>>
>> 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