CMS parallel initial mark

[Thomas Schatzl]

Hi,

On Fri, 2013-06-07 at 11:17 -0700, Jon Masamitsu wrote:
> On 6/7/2013 4:23 AM, Thomas Schatzl wrote:
> > Hi all,
> >
> >    some notes about the second patch, the modified eden boundary
> > sampling; in general I think the idea is good (Jon suggested that it
> > should become default), but imho there are some issues with the
> > implementation.
> >
> > On Tue, 2013-05-28 at 17:24 -0700, Hiroshi Yamauchi wrote:
> >> Hi,
> >>
> >> Here's a suggested patch in a separate patch:
> >>
> >>    http://cr.openjdk.java.net/~hiroshi/webrevs/edenchunks/webrev.00/
> >>
> >> that attempts to improve on this issue by implementing an alternative
> >> way of dividing up the eden into chunks for an increased parallelism
> >> (or better load balancing between the GC threads) for the young gen
> >> scan portion of the remark phase (and the now-parallelized initial
> >> mark phase.) It uses a CAS-based mechanism that samples the object
> >> boundaries in the eden space on the slow allocation code paths (eg. at
> >> the TLAB refill and large object allocation times) at all times.
> > - this patch is for DefNew/CMS only it seems. Is this correct?
> >
> > - in the Hotspot code base explicit != NULL checks are done for whatever
> > reason. To keep the same style it would be nice to update the checks
> > whether to do the sampling ("if (CMSEdenChunksRecordAlways &&
> > _next_gen)") accordingly.
> >
> > (the next point is not an issue of the patch)
> > - the check whether there is a _next_gen is odd - I do not think that
> > DefNew works as a generation without an older generation, but I'm not
> > sure.
> You're correct that DefNew needs to have a _next_gen.
> 
> > Looking at other similar code checking for _next_gen != NULL, the
> > response is typically to update _next_gen and then asserting that
> > _next_gen is != NULL. E.g.
> >
> >    if (_next_gen == NULL) {
> >      GenCollectedHeap* gch = GenCollectedHeap::heap();
> >      _next_gen = gch->next_gen(this);
> >      assert(_next_gen != NULL,
> >             "This must be the youngest gen, and not the only gen");
> >    }
> >
> > Jon?
> 
> Yes, except in a very few situations, _next_gen should be set.
> Exceptions I would expect is during initialization.  At the point
> Thomas indicates and assertion would be sufficient.
> 
> assert(_next_gen != NULL, "...");

Thanks.

> 
> 
> >
> > - I think CMSCollector::_eden_chunk_sampling_active should be volatile
> > to avoid any clever compilers skipping the read in sample_eden_chunk().
> > (Although I don't think it's possible here). See also below for avoiding
> > that variable completely.
> 
> Could you explain you thinking for making it volatile.  As you have, I 
> didn't think
> it was necessary and thought ,"If not necessary, don't do it".

It's not necessary I think, just as a reminder to the compiler to not
optimize that load away. I think the Atomic instruction contain the
necessary instructions though.

> > (btw, throughout this code there is a lot of checking whether some
> > NEW_C_HEAP_ARRAY returned NULL or not, and does elaborate recovery -
> > however by default NEW_C_HEAP_ARRAY fails with an OOM anyway... - but
> > that is certainly out of scope)
> 
> Thomas, could you file a CR for fixing that?  Thanks.

CR 8016276: CMS concurrentMarkSweepGeneration contains lots of
unnecessary allocation failure handling

> > Same for checking whether _survivor_chunk_array is NULL - at the moment
> > sometimes _survivor_chunk_array is checked against NULL to avoid
> > entering code (in print_eden_and_survivor_chunk_arrays()).
> >
> > The condition that could be used is the one that triggers allocation of
> > _survivor_chunk_array: (CMSParallelRemarkEnabled &&
> > CMSParallelSurvivorRemarkEnabled) in
> > concurrentMarkSweepGeneration.cpp:728.
> 
> Personally, I like a check like if (_eden_chunk_array != NULL)
> better. I think it is more future proof.   For whatever reason we don't
> try to dereference _eden_chunk_array if it is NULL.  An assertion
> checking that
> 
> (CMSParallelRemarkEnabled &&
> CMSParallelSurvivorRemarkEnabled)
> 
> is true would we be appropriate.

Also good.

Thomas