RFR: 8072809 - Clean up gen_process_roots() after generation array removal

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

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

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?

is_moving_collection can be merged with is_adjust_phase, I don't really 
have any opinion on which variable to keep.



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