RFR: JDK-8077842 - Remove the level parameter passed around in GenCollectedHeap

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Tue Jun 2 17:56:25 UTC 2015


Hi Kim!

I removed the unrelated formatting changes as suggested to minimize the 
conflicts with Bill's change. There will still be a conflict though since there 
were some actual changes as well.

I also added the is_young_gen()/is_old_gen() in GenCollectedHeap as suggested. 
There is as you mentioned no way to determine which generation it is inside the 
generation. I haven't found any good way to implement that, and I'm also not 
sure I want to since I think that the generation shouldn't need to know this... 
I may be wrong though.

New version:
http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.05/

Increment:
http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.05.incremental/

Thanks,
/Jesper


Kim Barrett skrev den 1/6/15 19:38:
> On May 28, 2015, at 11:57 AM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
>>
>> Hi,
>>
>> Another non-trivial merge later the webrev looks like this:
>>
>> http://cr.openjdk.java.net/~jwilhelm/8077842/webrev.04
>>
>> I reran all the tests and it turned out that the new assert in defNewGeneration.cpp was assuming that the generations were set up, which was not true at startup. So I have changed the assert to:
>>
>> +DefNewGeneration::IsAliveClosure::IsAliveClosure(Generation* young_gen) : _young_gen(young_gen) {
>> +  assert(_young_gen->kind() == Generation::ParNew ||
>> +         _young_gen->kind() == Generation::DefNew, "Expected the young generation here");
>>
>> This is more like the old assert that also looked at the properties of the generation itself:
>>
>> -DefNewGeneration::IsAliveClosure::IsAliveClosure(Generation* g) : _g(g) {
>> -  assert(g->level() == 0, "Optimized for youngest gen.");
>>
>> Compared to the other webrevs this is the only change modulo some code that was removed by recent changes and didn't need to be changed any more.
>>
>> Still need a Reviewer to have a look at this.
>>
>> Thanks,
>> /Jesper
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
>
> Much of the reformatting around lines 804-814 will collide with Bill's
> addition of spaces around xxx_FORMAT macros.  It might be simpler for
> which ever of you pushes second if unrelated reformatting were not
> part of this change set.
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp
> 5645   bool current_is_young = (current_generation == GenCollectedHeap::heap()->young_gen());
>
> Mikael suggested using the Generation::Type enum here.  There are
> quite a few expressions like this; maybe there should be is_young_gen
> and is_old_gen predicates?  To be similar in behavior they should
> perhaps be heap functions, e.g. heap->is_young_gen(generation).  But
> since old/young gens are each singletons, could instead be intrinsic
> to the generation, e.g. generation->is_young_gen().  I don't see
> anything that directly allows determination of the Generation::Type of
> a generation though.  And maybe that's intentional, as part of moving
> away from anything like "level"?
>
> ------------------------------------------------------------------------------
>


More information about the hotspot-gc-dev mailing list