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

[Mikael Gerdin]

Jesper,

On 2015-10-21 17:38, Jesper Wilhelmsson wrote:
> 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.

Thanks.

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

Looks good.
/Mikael

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