RFR [1/7]: 8197569: Refactor eager reclaim for concurrent remembered set rebuilding

[sangheon.kim]

Hi Thomas,

Thank you for these bunch of patches for rebuilding RemSet concurrently.
This is really nice.

On 03/06/2018 05:17 AM, Thomas Schatzl wrote:
> Hi Stefan,
>
>    thanks for looking into this.
>
> On Mon, 2018-03-05 at 16:11 +0100, Stefan Johansson wrote:
>> Thanks for splitting this change up Thomas,
>>
>> On 2018-03-05 15:18, Thomas Schatzl wrote:
>>> Hi all,
>>>
>>>     can I have some reviews for the following change that refactors
>>> the
>>> eager reclaim condition so that it can be later used for the policy
>>> that determines for which regions G1 should rebuild the remembered
>>> set?
>>> (And it's a reasonable refactoring on its own btw).
>>>
>>> CR:
>>> https://bugs.openjdk.java.net/browse/JDK-8197569
>>> Webrev:
>>> http://cr.openjdk.java.net/~tschatzl/8197569/webrev/
>> Look good, just one thing:
>> src/hotspot/share/gc/g1/g1CollectedHeap.cpp
>> 2542   bool humongous_region_is_candidate(G1CollectedHeap* heap,
>> HeapRegion* region) const {
>> ...
>> 2594     return obj->is_typeArray() &&
>> 2595 G1CollectedHeap::heap()-
>>> is_potential_eager_reclaim_candidate(region);
>> You could use the passed in heap parameter instead of using
>> G1CollectedHeap::heap().
> Done. Also renamed the parameter to "g1h" as used everywhere else.
Good but still there are some places using 'g1'. It would be good idea 
to fix, of course in other CR.

>
>> Another, maybe even nicer, solution would be to
>> move is_potential_eager_reclaim_candidate() to HeapRegion, and just
>> call region->is_potential_eager_reclaim_candidate().
> I did not do this for the following reasons:
>
> - is_potential_eager_reclaim_candidate may depend on global state in
> the future. E.g. when trying to eagerly reclaim objArrays or similar the result depends on whether we are currently marking or not.
>
> I do not like making HeapRegion dependent on e.g. G1CollectorState :)
> (any more than it already may be).
>
> - (almost) all eager reclaim related code has until now been in
> G1CollectedHeap. Such a change would go against this, and run kind of
> contrary to my intent to move away from using HeapRegion as dumping
> ground for anything loosely related to it out of convenience, but only
> as a representation for that memory.
>
> Either way is kind of fine for me right now, so let's see what others
> say?
Sounds reasonable to me.

>
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197569/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8197569/webrev.1/ (full)
webrev.1 seems good to me.

Thanks,
Sangheon


>
> Thanks,
>    Thomas
>