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

Dmitry Fazunenko dmitry.fazunenko at oracle.com
Tue Mar 17 17:11:24 UTC 2015


Hi Mikael,

Sorry for boring question, but how did you test the fix?
This change doesn't look like a trivial one, so how can you guarantee 
that it will not break anything?

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

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