RFR (M) 8075210: Refactor strong root processing in order to allow G1 to evolve separately from GenCollectedHeap

Mikael Gerdin mikael.gerdin at oracle.com
Tue Mar 17 16:00:59 UTC 2015

On 2015-03-17 15:26, Thomas Schatzl wrote:
> Hi Mikael,
>    some comments for the full change:
> On Mon, 2015-03-16 at 14:37 +0100, Mikael Gerdin wrote:
>> Hi all!
>> Currently SharedHeap::process_strong_roots is called both by the
>> GenCollectedHeap based collectors (CMS, Serial) and G1.
> [...]
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8075210
>> Webrevs:
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-1/webrev
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-2/webrev
>> http://cr.openjdk.java.net/~mgerdin/8075210/phase-3/webrev
>> Full webrev:
>> http://cr.openjdk.java.net/~mgerdin/8075210/full/webrev
> - I think allocation.inline.hpp does not seem to be required in the
> include list for the hpp file (but in the cpp file).


> - class forward declarations for G1CollectorPolicy, G1RemSet,
> ReferenceProcessor are not required. Ones (or includes) for the various
> closures are missing.


> - is it possible to add at least a few lines of documentation to the
> public methods of G1RootProcessor? To me it is not evidently clear what
> e.g. the difference between evacuate_roots, and process_strong_roots is.
> Also, G1CollectedHeap::g1_process_roots() did have at least some
> documentation.
> Or what G1RootProcessor actually is supposed to do.
> Please also mention that the G1RootProcessor is a scoped object that
> does some serious work in the constructor/destructor. I almost suggested
> (again :)) to just remove the StrongRootScope :)

I added a few comments in g1RootProcessor.hpp

I also want to get rid of StrongRootsScope, but I'm unsure of what to 
replace it with?

> - g1RootProcessor.?pp copyrights are from 2014.


> - in g1collectedheap.cpp:lines 4489-4492 the parameters for the call to
> evacuate_roots() are not aligned properly.

Fixed (not visible in webrev, see patch file).

> - the comment in line 5501-5505 in g1CollectedHeap.cpp is outdated,
> mentioning the StrongRootsScope object that moved to the
> g1rootprocessor.

Updated the comment.

> - there should imo be a warning in G1CollectedHeap::set_n_termination()
> why this is an empty method.

I don't think it's needed at all, I removed it.

> - I think Bengt already mentioned this, but g1CollectedHeap.cpp:98 still
> mentions g1_process_roots(). The entire paragraph seems outdated now. I
> do not think anything noted there is true any more.

I removed the entire comment. It seemed weird to keep the part about 
ParVerifyTask when no other parallel tasks were being described.

> - comment in g1CollectedHeap.hpp:1007+ is outdated.


> - please add braces for every if-statement in
> GenCollectedHeap::process_roots(). In the G1RootProcessor this has been
> done.


> - I think the instantiation of G1RootsProcessor in
> G1CollectedHeap::verify() should be scoped, i.e. it and the call to
> process_all_roots() enclosed with braces.


> I will need another round because I am somewhat confused about the
> changes regarding setting the number of threads in the various
> locations.

No problem.

Here's an incremental webrev incorporating your suggestions:
It's based on
which incorporates Eric's renaming request.

New full webrev at:


> Thanks,
>    Thomas

More information about the hotspot-gc-dev mailing list