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

[Mikael Gerdin]

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


--
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