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

[Mikael Gerdin]

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/

/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