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

[Stefan Johansson]

On 2018-03-06 14:17, 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.
>
>> 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?
I'm fine with this as well. Consider it reviewed :)

Stefan
> New webrevs:
> http://cr.openjdk.java.net/~tschatzl/8197569/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8197569/webrev.1/ (full)
>
> Thanks,
>    Thomas
>