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

[Tom Benson]

Hi Jesper,
No, not necessary.  Thanks -
Tom

On 11/20/2015 8:55 AM, Jesper Wilhelmsson wrote:
> Hi Tom,
>
> Thanks for reviewing!
>
> I fixed the comments. Let me know if you want to re-review this change.
>
> Thanks,
> /Jesper
>
>
> Den 19/11/15 kl. 23:28, skrev 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
>>>>>>>
>>>>>
>>>
>>