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

[Bengt Rutisson]

Hi Kim,

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.

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.


Some minor comments that you can feel free to ignore. I won't insist on 
any of them:

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".


g1CollectedHeap.cpp

1942   _in_cset_fast_test.initialize(
1943     _hrm.reserved().start(), _hrm.reserved().end(), 
HeapRegion::GrainBytes);
1944   _humongous_reclaim_candidates.initialize(
1945     _hrm.reserved().start(), _hrm.reserved().end(), 
HeapRegion::GrainBytes);

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

_in_cset_fast_test.initialize(_hrm.reserved().start(),
                               _hrm.reserved().end(),
HeapRegion::GrainBytes);
_humongous_reclaim_candidates.initialize(_hrm.reserved().start(),
_hrm.reserved().end(),
HeapRegion::GrainBytes);

See for example the call to 
JavaThread::satb_mark_queue_set().initialize() a few lines below this code.


This comment in humongous_region_is_candidate() reads a bit strange to me.

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.


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. But there 
shouldn't be any changes to g1_globals.hpp, right?

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

Thanks,
Bengt



>
> Incremental webrev:
> http://cr.openjdk.java.net/~kbarrett/8069367/webrev.04.incr/
> The incremental webrev might be a little misleading, since the base
> for it is not the version from the previous round of review, but
> rather that version plus the merge for 8076265.
>
> Testing:
> Hand testing
> JPRT
> Aurora Ad-hoc GC Nightly using G1
> local RefWorkload using G1
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150413/31322c75/attachment.htm>