CMS parallel initial mark

[Thomas Schatzl]

Hi,

On Tue, 2013-05-28 at 17:24 -0700, Hiroshi Yamauchi wrote:
> Hi,
> 
> I'd like to have the following contributed if it makes sense.
> 
> 1) Here's a patch (against a recent revision of the hsx/hotspot-gc repo):
> 
>   http://cr.openjdk.java.net/~hiroshi/webrevs/cmsparinitmark/webrev.00/
> 
> that implements a parallel version of the initial mark phase of the
> CMS collector. It's relatively a straightforward parallelization of
> the existing single-threaded code. With the above patch, I see about
> ~3-6x speedup in the initial mark pause times.

  had a look at this patch in the version at 
http://cr.openjdk.java.net/~hiroshi/webrevs/cmsparinitmark/webrev.01/

Looks good so far, some comments:
  - in cmsOopClosures.hpp, MarkRefsIntoClosure and the new
Par_MarkRefsIntoClosure could be refactored slightly as they have
exactly the same member variables. Not sure how this situation is
handled in other code though, and what others (Jon) 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).

 - I think Jon already mentioned that it might be better to use an int
for worker_id in CMSParMarkTask::do_young_space_rescan() (if it is
actually needed) to keep that code in line with current use.

 - the comments for at least CMSCollector::
initialize_sequential_subtasks_for_young_gen_rescan() and
CMSCollector::merge_survivor_plab_arrays() should be reworked - they
only mention parallel rescan as entry points.

 - the change in the assert in
CMSCollector::merge_survivor_plab_arrays() looks a little odd now. I
mean, looking at that assert in isolation poses the question why is this
method only called when doing initial mark in parallel? It somehow seems
that the same work is done somewhere else just for the initial mark and
serial case, which just looks strange to me. That's just an observation
from me.

assert(_collectorState == FinalMarking ||
         (CMSParallelInitialMarkEnabled && _collectorState ==
InitialMarking), "Error");

 - in CMSCollector::CMSCollector() there is this unconditional setup of
a few data structures used for parallel processing if any of the
CMSParallel* switches is enabled. 

Hth and thanks for the contribution :),
Thomas