RFR: 8072809 - Further cleanups after Generation array removal: gen_process_roots and OopsInGenClosure::set_generation

[Jesper Wilhelmsson]

Hi Mikael,

Den 19/10/15 kl. 09:28, skrev 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 renamed the bug as you can see it the subject. I also added information about 
the new cleanup in the bug.

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

Fixed.

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

Thanks,
/Jesper

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