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

[Jesper Wilhelmsson]

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