RFR (M) 8138762: Refactor setup of evacuation closures in G1

[Erik Helin]

On 2015-10-08, Mikael Gerdin wrote:
> Hi Erik,
> 
> On 2015-10-06 15:36, Erik Helin wrote:
> >Hi Mikael,
> >
> >first of all, thanks for a really nice patch! I think the patch
> >essentially is good to go, just a few comments inline.
> >
> >On 2015-10-02, Mikael Gerdin wrote:
> >>I've split the change into two different webrevs.
> >>The first one moves a bunch of code out from g1CollectedHeap.cpp to become
> >>externally visible.
> >>I also noticed that g1CodeBlobClosure.hpp is missing include guards.
> >>To make room for the closure set in the par scan state I remove the unused
> >>g1 rem set pointer in this first webrev.
> >>
> >>http://cr.openjdk.java.net/~mgerdin/8138762_1/webrev.0/
> >
> >Looks good, Reviewed.
> >
> >>The second webrev contains the actual change.
> >>I decided to separate the interface headers and the actual implementations
> >>of the closure sets in two separate files.
> >>g1RootClosureSet.hpp contains the interface and g1EvacuationClosures.hpp
> >>contains the implementations used by a G1 evacuation pause.
> >>
> >>http://cr.openjdk.java.net/~mgerdin/8138762_2/webrev.0/
> >
> >- g1CollectedHeap.cpp:
> >   The variable trace_metadata can be initialized with the
> >   expression:
> >   `bool trace_metadata =
> >     _g1h->collector_state()->during_initial_mark_pause() &&
> >     ClassUnloadingWithConcurrentMark;`
> 
> Ok.
> 
> >
> >- g1RootProcessor.cpp:
> >   Could you update the assert comments for:
> >   +    assert(closures->second_pass_weak_clds() != NULL, "should be
> >   null");
> >   +    assert(closures->second_pass_weak_clds() == NULL, "should not
> >   null");
> 
> Oops, yes of course.
> 
> >
> >- g1RootClosureSet.hpp:
> >   Could you rename the interface (and the file) to g1RootClosures?
> >   Correspondingly, rename G1EvacuationRootClosureSet to
> >   G1RootEvacuationClosures?
> 
> We agreed on G1EvacuationRootClosures.
> 
> >
> >- g1EvacuationClosures.hpp:
> >   Rename G1ClosureSet to G1SharedClosures?
> >   I'm not too fond of having comments before bool arguments,
> >   like /* process_only_dirty_klasses */. Personally I would remove them,
> >   but if you want to keep them, that is fine by me as well.
> 
> I renamed G1ClosureSet, I'd prefer to keep the comments because otherwise
> the code is hard to follow, especially since the shared closures constructor
> uses two different booleans.
> 
> I also noticed that accidentally passed the wrong closure to
> ref_processor_cm()->weak_oops_do. That shouldn't actually matter since the
> strong vs weak differentiation is only relevant during initial mark (and at
> that point the CM ref processor should not have discovered any references)
> but I'd prefer to maintain the same semantics instead of cleaning that up at
> this point.
> 
> 
> Incremental webrev at:
> http://cr.openjdk.java.net/~mgerdin/8138762_2/webrev.0_to_1/
> 
> Full webrev at:
> http://cr.openjdk.java.net/~mgerdin/8138762_full/webrev.1/

Looks good, Reviewed.

Thanks,
Erik

> /Mikael
> 
> >
> >Thanks,
> >Erik
> >
> >>The full webrev is available at:
> >>http://cr.openjdk.java.net/~mgerdin/8138762_full/webrev.0/
> >>
> >>Testing:
> >>JPRT
> >>Performance (aurora.se G1 workloads on Solaris x64)
> >>Kitchensink,runThese,vm.gc.testlist,vm.g1classunload.testlist
> >>
> >>I appreciate any feedback, if you have suggestions for different class names
> >>I'm happy to change them, I had some trouble coming up with good ones.
> >>
> >>Thanks
> >>/Mikael
>