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

[Mikael Gerdin]

Hi Dima,

On 2015-03-17 18:11, Dmitry Fazunenko wrote:
> Hi Mikael,
>
> Sorry for boring question, but how did you test the fix?

It's not a boring question!
I've been running the phase-1 part of the change when developing a 
separate feature. I've also tested the change with JPRT, which found 
some bugs in an earlier version of the pathces.

> This change doesn't look like a trivial one, so how can you guarantee
> that it will not break anything?

I've tested the G1 change in two parts:
In phase-1 I changed the standard GC pauses to use the new code while 
having the verification code call the old variant. Hopefully this shows 
that the new code in G1RootProcessor does not break anything.

In phase-2 I switched the verification code to use the new code as well, 
as I was satisfied that it was performing correctly.

In phase-3 I simply cut-pasted a bunch of code to another class in order 
to clean it up.

>
> I know you guys are monsters and you can verify changes just looking at
> webrev. But what about simple unit tests verifying basic invariants?

There are no basic invariants.
This code is inherently stateful and touches a lot of global objects.
Without a framework for mocking the entire JVM around G1RootProcessor I 
have a hard time figuring out how to test it.

/Mikael

>
> Thanks,
> Dima
>
> On 17.03.2015 19:00, Mikael Gerdin wrote:
>> 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
>>>
>>>
>>
>