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

[Kim Barrett]

On Mar 9, 2015, at 5:52 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi Kim,
> 
>  thanks for looking into this issue…

Thanks for all your help with this.

> On Fri, 2015-03-06 at 13:10 -0500, Kim Barrett wrote:
>> Please review this change to fix a problem in the interaction between
>> G1 concurrent marking and eager reclaim of humongous objects.
>> 
>> I will need a sponsor for this change.
> 
> I can sponsor it.

Thanks.

>> […]
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8069367
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8069367/webrev.00/
> 
> - concurrentMark.inline.hpp: in is_stale_humongous_queue_entry(), in the
> long comment, could the change add some text that mentions that "Since
> the humongous object has already been reclaimed, we can only check that
> its base address is region aligned.”.

OK.

> - please improve the asserts by adding the failing addresses in question
> (and region index). This helps diagnosing issues.

OK.

> - it's "humongous object" and not "humongous" too in the assert.

OK.

> - concurrentMark.hpp: the _stale_humongous_queue_entries should be a
> size_t. Sangheon has reviews for the patch that changes the other
> related members from int's already. So maybe wait until it has been
> pushed for posting the new webrev.

Yes, I saw that review request.  Looks like it’s been pushed.

> - concurrentMark.cpp: what do you think about refactoring the 
> 
>  is_stale_humongous_queue_entry(obj) {
>    // ignore
>  } else {
>    // do some asserts
>    scan_object();
>  }
> 
> into an extra method? This would allow unification of the asserts too.
> The "assert(...->is_marked(obj), ...)" is duplicated in
> CMTask::do_marking_step(), scan_object() already does that. Also the
> master_free_list assert could be moved into the scan_object() method I
> think. I cannot see a case where this would not be the case.

I was trying to minimize the changes, probably overly so.  I’ve rearranged
the asserts a bit and added a helper function.  Note that it does not
directly include the code for is_stale_… and eliminate that predicate;
it turns out there’s another place that needs that predicate, in
VerifyNoCSetOopsClosure.

> Also I would personally prefer to switch the if- and else- parts of that
> (invert the condition). When reading if-statements, I somewhat expect
> the far common case of execution to not be put into the else-part. It's
> your call.

I see it as a filter with the base case being the scan.  Writing it that way also
avoids needing to negate the predicate result, which I find easier to read.
This *is* a place where I might wish for an UNLIKELY-style macro.

> Please add the test case used to reproduce that explicitly enables eager
> reclamation. While failure without the change is not 100% reproducable,
> it seems good enough.

It was somewhat difficult to produce, but I’ve now got a reduced test case that
usually fails pretty quickly on two quite different machine configurations.  It will
be included in the next webrev.