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

Wed Oct 14 11:04:10 UTC 2015

On 2015-10-13 14:02, Mikael Gerdin wrote:
> All,
>
> I did an on-site review with StefanK and I have some new webrevs.
>
> To try to keep it simple to see the changes I've split them up into
> several incremental webrevs but I still intend to push them as a single
> change once the review is completed.
>
> --
> http://cr.openjdk.java.net/~mgerdin/8138762_2/stefan1/
> g1ParScanThreadState.cpp:
> * Moved factory method to become a static member of
> G1EvacuationRootClosures.
>
> g1RootClosures.hpp:
> * Moved second_pass_weak_clds down the class hierarchy to
> G1EvacuationRootClosures.
> * Removed thread_root_codeblobs since it is actually the same as
> strong_codeblobs.
> * Removed weak_codeblobs.
>
> g1RootProcessor.cpp:
> * After removing the weak_codeblobs accessor I moved the call to
> CodeCache::blobs_do back to process_all_roots.
> * Added some commments describing why AllRootsClosures visits all roots.
>
> g1EvacuationClosures.hpp became g1RootClosures.cpp
> * The concrete types are no longer exposed to the rest of the code.
> * Removed some superfluous "virtual" qualifiers.
> * Added factory method for the concrete types implementing
> G1EvacuationRootClosures.
>
> --
> http://cr.openjdk.java.net/~mgerdin/8138762_2/stefan2/webrev/
> g1CollectedHeap.cpp / g1RemSet.cpp / g1RemSet.hpp
> Moved the allocation of the code blob closure out from G1RemSet.
>
> g1RootClosures.cpp / g1RootClosures.hpp
> re-introduce weak_codeblobs to pass a code blob closure to the code
> cache rem set scanning.
> remove raw_weak_oops since it was only needed to create the code blob
> closure in G1RemSet.
>
> --
> http://cr.openjdk.java.net/~mgerdin/8138762_2/trace-metadata/webrev/
> Remove the trace_metadata boolean parameter to evacuate_roots and make
> it into a member function on the G1EvacuationRootClosures interface.
> This keeps the knowledge of initial mark closures' complexity in
> g1RootClosures.cpp

And here is a webrev of all the changes (minus 8138762_1/webrev)
including trace-metadata but excluding parpush:

http://cr.openjdk.java.net/~mgerdin/8138762_2/webrev.2/

/Mikael

>
>
> --
> http://cr.openjdk.java.net/~mgerdin/8138762_2/parpush/webrev/
> As a bonus I tried converting oops_into_collection_set_do use the
> closure holder object as well and moved the G1ParPushHeapRSClosure into
> G1EvacuationRootClosures.
> I kind of like that it reduces the amount of places where
> G1ParPushHeapRSClosure is mentioned but then again this might be just
> "changing code" and not making it better :)
>
>
> /Mikael
>
> On 2015-10-02 17:29, Mikael Gerdin wrote:
>> Hi all,
>>
>> I've grown tired of the way the closures are set up in G1ParTask, where
>> 3 different sets of closures are allocated on the stack and then
>> pointers to them are passed to the root processor depending on the type
>> of GC we are doing.
>>
>> My suggested solution to this is to introduce an interface for a policy
>> object which is queried by the root processor for different closures:
>>    32 class G1RootClosureSet : public CHeapObj<mtGC> {
>>    33
>>    34 public:
>>    35   // Closures to process raw oops in the root set.
>>    36   virtual OopClosure* weak_oops() = 0;
>>    37   virtual OopClosure* strong_oops() = 0;
>>    38
>>    39   // Closures to process CLDs in the root set.
>>    40   virtual CLDClosure* weak_clds() = 0;
>>    41   virtual CLDClosure* strong_clds() = 0;
>>    42
>>    43   // Applied to the CLDs reachable from the thread stacks.
>>    44   virtual CLDClosure* thread_root_clds() = 0;
>>    45   // Applied to the weakly reachable CLDs when all strongly
>> reachable
>>    46   // CLDs are guaranteed to have been processed.
>>    47   virtual CLDClosure* second_pass_weak_clds() = 0;
>>    48
>>    49   virtual CodeBlobClosure* strong_codeblobs() = 0;
>>    50   virtual CodeBlobClosure* weak_codeblobs() = 0;
>>    51   // Applied to the code blobs reachable from the thread stacks.
>>    52   virtual CodeBlobClosure* thread_root_codeblobs() = 0;
>>    53 };
>>
>> This policy object is then instantiated by a factory method which
>> decides on which implementation to create depending on the type of the
>> current gc. The policy object is stored in the par scan thread state and
>> is used later on when the reference processing code use the same
>> closures.
>>
>> For G1MarkSweep and verification I've kept the external api of the root
>> processor the same and wrap the closures before passing them on inside
>> the root processor.
>>
>> --
>>
>> 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/
>>
>> --
>>
>> 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/
>>
>> --
>>
>> 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
>