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

[Thomas Schatzl]

Hi,

On Tue, 2015-03-10 at 19:23 -0400, Kim Barrett wrote:
> On Mar 9, 2015, at 5:52 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> > 
> > Hi Kim,
> > 
[...]
> > - 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.

Yes :)

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

Okay.

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

I am not insisting on changing this.

I did some experiments with a LIKELY/UNLIKELY macro just recently (for
8060025) on gcc, and I was only able to make performance worse (in the
memory allocation part of the code).

This mechanism seems way too brittle to be really useful: also
considering that it most likely requires regular maintenance.

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

Great :) Looking forward to it.

Thanks,
  Thomas