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

Tue Apr 14 09:05:49 UTC 2015

Hi Kim,

On 2015-04-14 04:25, Kim Barrett wrote:
> 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.

Good choice to just avoid the long line. I saw the other emails about 
this too. I agree with you that readability is more important than 
slavishly following a style guide (especially if it is not written 
down). And ,as you note, readability is not easily agreed upon. As I 
stated in my previous email all of my comments for your patch are free 
to ignore. I don't have strong opinions about them. Overall the patch 
looks good.

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

Do yo mean that this is the paragraph that explains the liveness issue?

3436     // In order to maintain SATB invariants, during concurrent mark
3437     // we should only nominate an object containing references if it
3438     // was allocated after the start of marking, as such an object
3439     // doesn't need to have its references scanned.

I interpret that as that we should not nominate the objects that we 
actually then go ahead and nominate anyway. And that the comment 
starting at line 3445 just says that we do this. I don't understand this 
comment (lines 3436-3439) as explaining that the main problem is that 
reclaimable humongous objects can be the only old objects that keep an 
object graph alive. Can we reword this somehow? I've read the full 
comment a few times and to me it still sounds like the main problem is 
the remembered sets, which I don't really agree with. I agree that it is 
a problem, but I don't think it is the main problem.

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

Ok. Good. I was just wondering.

Thanks,
Bengt

>