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

[Thomas Schatzl]

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?

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