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

[Kim Barrett]

On Mar 9, 2015, at 9:59 AM, Bengt Rutisson <bengt.rutisson at oracle.com> wrote:
> 
> One request for the webrev though. The method CMTask::is_stale_humongous_queue_entry() does not check that obj is a humongous object and I don't think it has any chance of doing that either. It can just check that the object looks like it is newly allocated and that could only ahppen for reclaimed humongous objects. Given that I think I would prefer to not have the method called something with humongous. 
> 
> If we do what Thomas suggested to avoid code duplication, which was  to move these lines into a separate method:
> 
> 
> 3822 
> 3823       if (is_stale_humongous_queue_entry(obj)) {
> 3824         statsOnly( ++stale_humongous_queue_entries );
> 3825       } else {
> 3826         assert(!_g1h->is_on_master_free_list(
> 3827                  _g1h->heap_region_containing(obj)), "invariant");
> 3828         scan_object(obj);
> 3829       }
> 3830 
> 
> Then maybe we can just inline the code from is_stale_humongous_queue_entry() to avoid having to give it a somewhat confusing name.

I’ve added the helper function that Thomas suggested.  However, the is_stale… predicate
ended up not getting hoisted into it and eliminated as a named function.  I’m not sure I would
have done that anyway, but it turned out that predicate is needed in another place.

As for the name of the predicate, it was chosen to describe the semantics of the test being
performed.  Then there’s an implementation comment describing the “tricky” way it accomplishes
this.  If it was named according to it’s implementation then readers of the call sites would be left
scratching their heads about why we’re doing that.

I did end up renaming it slightly, and also moved it, because of the newly discovered caller. It’s
now ConcurrentMark::is_stale_humongous_marked_entry, which tests an entry from a
(local or global) mark stack for staleness.