RFR: 8072809 - Clean up gen_process_roots() after generation array removal

[Jesper Wilhelmsson]

Hi Mikael,

Thanks for reviewing!

I addressed all your comments and split the change into two. I'll send out a 
separate mail for the other changes: JDK-8139772 - Cleanups in Generation 
related code

New webrev is here:
http://cr.openjdk.java.net/~jwilhelm/8072809/webrev.01/

More comments inline.


Den 1/10/15 kl. 15:45, skrev Mikael Gerdin:
> Hi Jesper,
>
> On 2015-09-09 23:02, Jesper Wilhelmsson wrote:
>> Hi,
>>
>> The generation cleanup inspired several other cleanups. Please review a
>> few of them here.
>
> I find it a bit odd that the synopsis of the issue focuses on one cleanup fix
> but several others are involved in the suggested change.
>
> Would it be possible to split the *process_roots changes into a different
> changeset?
>
> I think all the non-root-processing changes look good but it's hard to get a
> handle on them since they are a bit spread out.
>
>>
>>
>> 1. gen_process_roots()
>> In the generation array world this method was used to process roots for
>> any given generation and did different things based on which generation
>> it processed. I split it into two methods, one for the young generation
>> and one for the old. This made the code a lot cleaner and easier to
>> read. Even though the method was "duplicated" the result is only five
>> lines of code longer, and this patch all in all actually removes more
>> lines than it adds. :)
>
> I think the split into young_process_roots and old_process_roots is a sane one
> but I do have some comments on the actual implementation:
>
> genCollectedHeap.cpp, young_process_roots:
>   677   if (!_process_strong_tasks->is_task_claimed(GCH_PS_younger_gens)) {
>   678     young_gen_closure->reset_generation();
>   679   }
> is a strange bit of code now that it's been decoupled from the old for(level =
> 0; ...) block
>
> It basically says that for a young gc, one gc thread should reset the generation
> of its root scan closure (the not_older_gens closure)
> The generation of that closure has not been changed at any other point so it's a
> pure no-op.

I removed the whole ability to change the generation in these closures and added 
asserts to verify that they never changed in the first place.

>
> I also think that the not_older_gens (which you renamed to young_gen_closure)
> should be simply named "root_closure" since it's the closure that is applied to
> all the roots.

Fixed.

>
>   672   MarkingCodeBlobClosure mark_code_closure(young_gen_closure, true /*
> young collections are always moving */);
> In the rest of the code we pass CodeBlobToOopClosure::FixRelocations instead of
> a raw "true".

Fixed.

>
> old_process_roots:
> young_gen_closure is used to scan all roots, both non-heap roots and the young
> generation (if young_gen_as_roots). Can you rename it to "root_closure" as I
> suggested for young_process_roots?

Fixed.

>
> is_moving_collection can be merged with is_adjust_phase, I don't really have any
> opinion on which variable to keep.

Fixed.

Thanks,
/Jesper

>
>
>
>>
>>
>> 2. Generation::spec()
>> Generation::spec() is only called in a few places and all of them are
>> really looking for the initial size of the generation. I replaced
>> Generation::spec() with Generation::initial_size().
>>
>>
>> 3. GenCollectedHeap::gc_stats()
>> Not only was this method unused, is was also completely pointless. Removed.
>>
>>
>> 4. Generation::full_collects_young_generation()
>> This method is only ever called in old generations, and all old
>> generations override it and return !ScavengeBeforeFullGC. The method was
>> removed and the flag is checked directly in the only place it was called.
>
> As mentioned above these are good changes but it would be nice to extract them
> from this patch.
>
> /Mikael
>
>>
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8072809
>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8072809/webrev.00/
>>
>> Testing: RBT vm.gc on all platforms.
>>
>> Thanks,
>> /Jesper
>