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

[Mikael Gerdin]

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

Fixed.

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

Fixed.

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

Fixed.

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

Removed.

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

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.

Fixed.

>
> 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:
http://cr.openjdk.java.net/~mgerdin/8075210/thomas-comments/webrev/
It's based on
http://cr.openjdk.java.net/~mgerdin/8075210/done_with_threads/webrev/
which incorporates Eric's renaming request.

New full webrev at:
http://cr.openjdk.java.net/~mgerdin/8075210/full2/webrev/

/Mikael

>
> Thanks,
>    Thomas
>
>