RFR(S): 8048179: Early reclaim of large objects that are referenced by a few objects

[Thomas Schatzl]

Hi Bengt,

On Wed, 2014-12-10 at 16:59 +0100, Bengt Rutisson wrote:
> Hi Thomas,
> 
> On 2014-12-10 15:44, Thomas Schatzl wrote:
> > Hi all,
> >
> >    can I have reviews for this small change that improves eager reclaim
> > efficiency at no (measureable) cost?
> >
> > Until that change we only ever considered humongous objects with
> > absolutely no references for eager reclaim. This change widens that
> > restriction by allowing humongous objects with a few references as
> > candidates.
> >
> > This works simply by pushing remembered set cards of such humongous
> > objects into the DCQS at the beginning of evacuation. The update rs
> > phase will automatically (and in parallel!) determine whether there is
> > still a reference to an object or not.
> >
> > Humongous objects that do not contain any remembered set entries after
> > gc (and the other conditions that prevent reclamation do not hold like
> > reference from young gen) that object is reclaimed.
> >
> > I arbitrarily defined "having a few references" as a region having only
> > sparse remembered set entries.
> >
> > There is a new global G1ReclaimDeadHumongousObjectsWithRefsAtYoungGC to
> > turn off this particular feature of early reclamation in case it takes
> > too long. The default value is true, i.e. it is turned on by default.
> >
> > All benchmarks we have looked at showed that there is no particular
> > pause time impact by this change if it has been ineffective, however
> > there is a new phase "Humongous Register" in the log that can be used to
> > determine the impact.
> >
> > Some benchmarks show some improvement in overall performance due to the
> > feature reclaiming temporary objects much faster.
> >
> > There is also some cleanup in the debug messages enabled by
> > G1TraceReclaimDeadHumongousObjectsAtYoungGC.
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8048179
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8048179/webrev.0/
> 
> Overall this looks good to me. Some minor comments:
> 
> - Could we name the tests a bit more creative than 
> TestEagerReclaimHumongousRegions, TestEagerReclaimHumongousRegions2 and 
> TestEagerReclaimHumongousRegions3? Maybe 
> TestEagerReclaimHumongousRegions, 
> TestEagerReclaimHumongousRegionsClearMarkBits, 
> TestEagerReclaimHumongousRegionsWithRefs ?

Fixed.

> 
> - I think it would be good to communicate that we are not reclaiming 
> humongous objects that actually have references. We reclaim those that 
> only have stale references. Maybe we can name the flag 
> G1ReclaimDeadHumongousObjectsWithStaleRefs and have the description be:
> 
>   277           "Try to reclaim dead large objects that have a few stale 
> "        \
>   278           "references at every young 
> GC.")                                  \
> 

Fixed.

> 
> If is_small_for_eager_reclaim() would return true also if 
> OtherRegionsTable::is_empty() is true then the farily complex check in 
> G1CollectedHeap::humongous_region_is_candidate() could be made simpler:
> 
> 3490   return !oop(region->bottom())->is_objArray() &&
> 3491          (G1ReclaimDeadHumongousObjectsWithRefsAtYoungGC && 
> region->rem_set()->is_small_for_eager_reclaim());
> 

We already talked about this: this has been a misunderstanding and the
current code is okay.

> Also, why is humongous_region_is_candidate() a public method on 
> G1CollectedHeap instead of a private method in 
> RegisterHumongousWithInCSetFastTestClosure?
> 

Fixed.

> I am also a little bit concerned with is_small_for_eager_reclaim(). It 
> kind of pushes implementation details about early reclamation down into 
> the remembered sets. I don't really have a good idea of how to design 
> it. But one question is if you have had a chance to measure the overhead 
> of just not filter at all and flush the remsets for all humongous objects?

The problem is that doing a "r->rem_set()->occupied() < X" is too slow.

I changed this to a different predicate called
occupancy_less_or_equal_than() of the remembered set which sounds a bit
more generic, but does basically the same thing, supporting only
comparing a given occupancy to the sparse remembered set entry count.

At least the code now does not know about the remembered set levels any
more.

Diff webrev:
http://cr.openjdk.java.net/~tschatzl/8048179/webrev.0_to_1/
Complete webrev:
http://cr.openjdk.java.net/~tschatzl/8048179/webrev.1/

Checked again with JPRT, and did some more performance testing of the
changeset with no noticeable impact.

Thanks,
  Thomas