RFR (M) 8138762: Refactor setup of evacuation closures in G1
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Oct 14 11:16:47 UTC 2015
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.
>
> 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?
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);
>
> 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?
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