RFR (M) 8075210: Refactor strong root processing in order to allow G1 to evolve separately from GenCollectedHeap
thomas.schatzl at oracle.com
Tue Mar 17 14:26:06 UTC 2015
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.
> 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
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 :)
- g1RootProcessor.?pp copyrights are from 2014.
- in g1collectedheap.cpp:lines 4489-4492 the parameters for the call to
evacuate_roots() are not aligned properly.
- the comment in line 5501-5505 in g1CollectedHeap.cpp is outdated,
mentioning the StrongRootsScope object that moved to the
- there should imo be a warning in G1CollectedHeap::set_n_termination()
why this is an empty method.
- 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.
- comment in g1CollectedHeap.hpp:1007+ is outdated.
- please add braces for every if-statement in
GenCollectedHeap::process_roots(). In the G1RootProcessor this has been
- 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
More information about the hotspot-gc-dev