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

[Bengt Rutisson]

Hi Thomas,

New webrev looks good.

Two questions:

What do you think about changing 
OtherRegionsTable::occupancy_less_or_equal_than() to be something like:

bool OtherRegionsTable::occupancy_less_or_equal_than(size_t limit) const {
   if (limit <= (size_t)G1RSetSparseRegionEntries) {
     return occ_coarse() == 0 && _first_all_fine_prts == NULL && 
occ_sparse() <= limit;
   } else {
     Unimplemented();
     // Current uses of this method only use values less than 
G1RSetSparseRegionEntries for the limit.
     // If we want to use it for larger values we need to implement the 
more expensive iteration over
     // the remembered set to check the limit.
     return false;
   }
}

I think the Unimplemented() check communicates that it is this method 
that is having a problem, not the caller of the method.

The second question is about the flag name. I realize that 
G1ReclaimDeadHumongousObjectsWithStaleRefsAtYoungGC is correct and that 
it is in analogy with G1ReclaimDeadHumongousObjectsAtYoungGC. But it is 
very long. Can we find some way of shortening it? I know it was me who 
suggested to include "WithStaleRefs" in the name...

Maybe the flags could be called G1EagerReclaimOfHumObjs and 
G1EagerReclaimOfHumObjsWithStaleRefs ?

Thanks,
Bengt



On 2014-12-15 10:52, Thomas Schatzl wrote:
> 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
>
>