CMS parallel initial mark

[Thomas Schatzl]

Hi Jon,

On Fri, 2013-06-07 at 07:56 -0700, Jon Masamitsu wrote:
> On 6/6/2013 12:25 PM, Thomas Schatzl wrote:
> > Hi all,
> >
> > On Thu, 2013-06-06 at 10:06 -0700, Jon Masamitsu wrote:
> >> On 6/6/13 9:02 AM, Thomas Schatzl wrote:
> >>> Hi,
> >>>
> >>> On Tue, 2013-05-28 at 17:24 -0700, Hiroshi Yamauchi wrote:
> >
> > (Maybe it is also possible to remove the underscore in the class name?
> > It's not exactly customary to have class names with underscores in the
> > Hotspot code - I think)
> >
> >>>     - in concurrentMarkSweepGeneration.cpp in
> >>> CMSCollector::checkpointRootsInitialWork() the new code seems a little
> >>> strange to me. I.e. roughly it does the following:
> >>>
> >>>     [...]
> >>>     if (CMSParallelInitialMarkEnabled) {
> >>>       [ set up parallel stuff ]
> >>>       if (n_workers > 1) {
> >>>         // do parallel thread
> >>>       } else {
> >>>         // do serial work using the current thread
> >>>       }
> >>>     } else {
> >>>       // do serial work
> >>>     }
> >>>
> >>> it does not feel good to have two serial paths here; maybe use the
> >>> serial/original code if n_workers == 1 and guarantee(n_workers > 1, ) in
> >>> the first part of the if? Or look if the old serial path could be
> >>> removed? (or the new one).
> >> Is the inner serial path one thread executing a single chunk that
> >> does all the work (i.e., executes the parallel code but by only
> >> one thread)?
> > Yes, that is in my opinion a little awkward. It would be nice if we had
> > only one serial initial marking code path to simplify
> > debugging/maintenance if there are issues. Removal of one or the other
> > (if so) probably depends on how different the performance between the
> > two paths is.
> 
> I would prefer not to remove the path that executes the parallel code
> with a single thread.  It is a tool for looking for performance anomalies
> in the parallel code.  I've used code like it (i.e., executes parallel 
> code in
> one thread in UseParallelGC and UseParNewGC) this week while looking
> for a performance problem.
> 
> The strictly serial path (which is the code we have today) would be nice
> to have for a while as a workaround for any problems.

Okay, fair enough.

> > If the old serial version (old code) is kept, this condition needs to be
> > adapted, as data structures within the if-block started at that line are
> > only used in the parallel case.
> 
> Where talking about this like
> 
>    if ((CMSParallelRemarkEnabled && CMSParallelSurvivorRemarkEnabled) || 
> CMSParallelInitialMarkEnabled) {
> 
> and you're saying the
> > this condition needs to be
> > adapted, as data structures within the if-block started at that line are
> > only used in the parallel case.
> Sorry, I still don't get it.  Are you saying the condition in the test is
> not correct?  By "parallel case" do you mean the case where
> more than one GC thread does the work?  Or are you counting
> one GC thread executing the parallel code as part of the
> "parallel case"?

Sorry for confusing you. This was just some suggestion for if one of the
serial paths is removed (e.g. in the sense of if we remove that, we need
to also remove this). Since it has been decided to keep both for now (I
think), this comment is obsolete.

Thanks,
Thomas