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

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Wed Mar 18 10:09:18 UTC 2015


Mikael,

Thank you for detailed explanation, my "curiosity" is satisfied now. The 
level of testing looks good to me.

I understand the problem of writing unit tests and I keep hoping that 
one day we have a mocking framework for GC.
And the more we are talking about it the sooner this day will come.

Thanks,
Dima



On 18.03.2015 12:44, Mikael Gerdin wrote:
> 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
>>>>
>>>>
>>>
>>




More information about the hotspot-gc-dev mailing list