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

[Tom Benson]

Hi Jesper,
The change looks good to me.  I noticed a couple of comment issues 
adjacent to your changes that you could consider fixing...

In genCollectedHeap.hpp, this should now be "young_gen_as_roots":

   374 // If "younger_gens_as_roots" is false, younger generations are

In genOopClosures.hpp, the comment about _gen_boundary floated up above 
set/assert_generation somehow:

   69 // Problem with static closures: must have _gen_boundary set at 
some point,
   70 // but cannot do this until after the heap is initialized.
   71 void set_generation(Generation* gen);
   72 void assert_generation(Generation* gen);
   73
    74 HeapWord* gen_boundary() { return _gen_boundary; }

Tom


On 10/27/2015 5:07 AM, Mikael Gerdin wrote:
> 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
>>>>>
>>>
>