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

[Mikael Gerdin]

Hi Jesper,

On 2015-10-16 18:28, Jesper Wilhelmsson wrote:
> 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/

I when I commented that set_generation could go away I didn't expect you 
to do it as part of this change, but seeing as the work is done I don't 
want to stop you.
I would however like to request that you rename the issue now that it's 
not only cleaning up gen_process_roots any more.

I have some small comments remaining:
in genCollectedHeap.cpp the following block is useless. There is no 
requirement in _process_strong_tasks that all tasks are claimed before 
all_tasks_completed is called.

675 if (!_process_strong_tasks->is_task_claimed(GCH_PS_younger_gens)) {
676 }


Otherwise the change looks good.
/Mikael

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