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

[Thomas Schatzl]

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

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

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

Thanks,
  Thomas