RFR (M) 8138762: Refactor setup of evacuation closures in G1
Mikael Gerdin
mikael.gerdin at oracle.com
Wed Oct 14 12:43:07 UTC 2015
Hi Stefan,
Thanks for the review!
On 2015-10-14 13:16, Stefan Karlsson wrote:
> Hi Mikael,
>
> 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.
>
> 176 AllRootsClosures(OopClosure* roots, CLDClosure* clds,
> CodeBlobClosure* blobs) :
> 177 _roots(roots), _clds(clds) {}
>
>
> The blobs parameter should be removed as well.
Done.
>
>>
>> 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.
>
> +#include "gc/g1/g1CollectedHeap.hpp"
> #include "gc/g1/g1CodeBlobClosure.hpp"
>
> Could you change the order of these includes?
Oops, sorry. Fixed.
>
>
> Other than that, this looks good.
>
>>
>> --
>> 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.
>
> The reason why we're using the weak_codeblob closure instead of the
> strong_codeblob closure instead that we don't want to use this set of
> nmethods as roots for the concurrent marking. It's not obvious from
> reading the code, so it would probably be good to add a comment about that.
>
> G1ParPushHeapRSClosure push_heap_rs_cl(_g1h, pss);
> size_t cards_scanned =
> _g1h->g1_rem_set()->oops_into_collection_set_do(&push_heap_rs_cl,
> - pss->closures()->raw_weak_oops(),
> + pss->closures()->weak_codeblobs(),
> worker_id);
>
I added the following comment:
// We pass a weak code blobs closure to the remembered set scanning
because we want to avoid
// treating the nmethods visited to act as roots for concurrent marking.
// We only want to make sure that the oops in the nmethods are adjusted
with regard to the
// objects copied by the current evacuation.
>
>>
>> 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.
>
> Other than that, this looks good.
>
>>
>> --
>> 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
>
> Looks good.
>
>>
>>
>> --
>> 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 :)
>
> I'm not sure about this one. It pushes the knowledge about the new
> closure collections down into the rembered set scanning code, which
> makes it more complicated than it would have to be.
>
> I would rather see that we split this function up into two separate
> functions, unless there really is a _significant_ performance reason to
> keep the combined.
>
> You see that the function is performing two separate tasks just by
> reading the comment:
>
> 81 // Invoke "blk->do_oop" on all pointers into the collection set
> 82 // from objects in regions outside the collection set (having
> 83 // invoked "blk->set_region" to set the "from" region correctly
> 84 // beforehand.)
> 85 //
> 86 // Apply non_heap_roots on the oops of the unmarked nmethods
> 87 // on the strong code roots list for each region in the
> 88 // collection set.
> 89 //
> 90 // The "worker_i" param is for the parallel case where the id
> 91 // of the worker thread calling this function can be helpful in
> 92 // partitioning the work to be done. It should be the same as
> 93 // the "i" passed to the calling thread's work(i) function.
> 94 // In the sequential case this param will be ignored.
> 95 //
> 96 // Returns the number of cards scanned while looking for pointers
> 97 // into the collection set.
> 98 size_t oops_into_collection_set_do(G1EvacuationRootClosures*
> closures, I would like to see one function performing lines 81-84 and
> another function performing lines 86-88. If this was split into two
> functions, I think the code will become clearer and we would get rid of
> the need to pass down the closure collection object. Can we defer this
> code change?
Sure, that's sort of why I made this into a "bonus" change :)
I took the liberty to add a cleanup task for this.
/Mikael
>
>
> Thanks,
> StefaK
>
>>
>>
>> /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
>>
>
More information about the hotspot-gc-dev
mailing list