RFR: 8069367: assert(_nextMarkBitMap->isMarked((HeapWord*) obj)) failed

[Kim Barrett]

On Apr 13, 2015, at 6:34 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> On 11/04/15 07:51, Kim Barrett wrote:
>> Another round.
>> 
>> I've updated my local repo from the team repo, merging with the
>> changes from 8076265: Simplify deal_with_reference.
>> 
>> I've backed out the G1EagerReclaimHumongousPreSnapshotTypeArrays
>> experimental configuration option, per Bengt's suggestion that it will
>> likely be more trouble than it's worth.
>> 
>> Because of the latter, the test to determine whether a humongous
>> region should be a candidate for eager reclamation has been
>> simplified, since at present there is no need to check the allocation
>> time of the object.  But I've left the commentary there about
>> different handling of objects containing references, and noting the
>> special handling of typeArray objects in this respect.
>> 
>> Also had to move some new inline function definitions from
>> g1CollectedHeap.hpp to g1CollectedHeap.inline.hpp, to account for
>> header cleanups since my repo was last updated.
>> 
>> CR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8069367
>>  
>> 
>> Webrev:
>> 
>> http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04/
> 
> The latest webrev looks ok to me. I'm fine with pushing it as is.

Bengt,

Thanks for looking at this.

> However, I think it would have been easier to review this if it had
> been split up to two changes. First a preparation fix that does the
> renaming of _humongous_is_live to _humongous_reclaim_candidates and
> the removal of clear_humongous_is_live_table() and the other
> refactoring. Then one that actually includes the fix, with
> process_grey_object() and the changes to
> CMTask::deal_with_reference().
> 
> These two things are separable, right? Or am I missing something? As I
> said I'm fine with pushing this as is, I just want to make sure that I
> have understood correctly.

In retrospect these probably are separable, though the re-factoring
probably would not have happened if not for the proposed bug fix.

> concurrentMark.inline.hpp
> The comment saying "Immediately process arrays of binary data" is a
> little odd to me. I think I would prefer something like "Immediately
> process arrays of primitive types".

Yes, that's better wording.  Making this change.

> When we line break long parameter lists we usually put one parameter
> per line. The above would be:

I find the suggested change harder to read, especially to notice the
two initialize() calls are being passed the same arguments.  So I made
a different change, capturing the values in variables, which makes the
calls fit on single lines of reasonable length.

> 3445     // However, we presently only nominate is_typeArray() objects.
> 3446     // A humongous object containing references induces remembered
> 3447     // set entries on other regions.  In order to reclaim such an
> 3448     // object, those remembered sets would need to be cleaned up.
> 
> I think the main problem with reclaiming objects that contain
> references is that they may be the only old objects that keep an
> object graph alive. If new objects reference any object in that object
> graph we have to be sure that they are kept alive. Otherwise we will
> have stale pointers in the new objects.

No, that problem is dealt with by the allocation age test discussed
earlier, in the paragraph about maintaining SATB invariants.  (Though
there's no code for it, since we don't need it at present.)

The problem being discussed in the paragraph in question is that if we
have a humongous object H containing a reference to object O in some
other region R, then R has a remembered set entry for H or some card
in H.  But if we reclaim H, then that remembered set entry in R now
refers a reclaimed region, and that presently causes problems.  Here's
the bug: https://bugs.openjdk.java.net/browse/JDK-8071913.

> g1_globals.hpp
> No changes. This is probably just webrev acting up. For some reason it
> includes files that have been modified and then reverted.

That's correct.

> TEST.groups
> Why is the test TestGreyReclaimedHumongousObjects excluded from the
> hotspot_gc target?

I was following a suggestion from Christian to exclude what is
essentially a stress test from JPRT tests (with -testset hotspot),
where we have a limited time budget (especially when pushing).  My
understanding is that it will still be run during nightly testing.