RFR(M) 8043575: Dynamically parallelize reference processing work

[Thomas Schatzl]

Hi Sangheon,

On Fri, 2018-06-01 at 14:48 -0700, sangheon.kim at oracle.com wrote:
> Hi all,
> 
> As webrev.0 is conflicting with webrev.0 of "8203319: JDK-8201487
> disabled too much queue balancing"(out for review, but not yet
> pushed), I'm posting webrev.1.
> 
> http://cr.openjdk.java.net/~sangheki/8043575/webrev.1
> http://cr.openjdk.java.net/~sangheki/8043575/webrev.1_to_0
> 

Some minor comments:

- g1ConcurrentMark.cpp:1521: maybe update the comment or remove it.

- g1FullGCReferenceProcessorExecutor.cpp: I think
G1FullGCReferenceProcessingExecutor::run_task(AbstractGangTask*) is not
used any more.

- I think that in line with not supporting this feature in CMS, the
assert in concurrentMarkSweepGeneration.cpp:5126 should not check >=
but ==? Same in parNewGeneration.cpp:796.

- gc_globals.hpp: I would prefer something like the following for the
text for ReferencesPerThread:

"Ergonomically start one thread for this amount of references for
reference processing if ParallelRefProcEnabled is true. Specify 0 to
disable and use all threads."

- would you mind renaming ReferenceProcessor::has_adjustable_queue? Not
the queues are adjusted, but the number of processing threads.

- since the feature changes the number of threads, it should also
naturally affect the need_balance_queues() method and impact the code
in process_discovered_reflist(). I think must_balance must be re-
evaluated after every decision to set the number of threads.

At the moment the number of threads is adjusted after deciding whether
we need balancing.

- the patch might actually be much simpler after JDK-8202845: Refactor
reference processing for improved parallelism. Since I do not want you
to spend time on fixing this change after I mess it up, would you mind
me taking over this work/CR?

Thanks,
  Thomas

> Thanks,
> Sangheon
> 
> 
> On 5/30/18 9:43 PM, sangheon.kim at oracle.com wrote:
> > Hi all,
> > 
> > Could I have some reviews for this patch?
> > 
> > This patch is suggesting ergonomically choosing worker thread count
> > from given reference count.
> > We have ParallelRefProcEnabled command-line option which enables to
> > use ALL workers during reference processing however this option has
> > a drawback when there's limited number of references. i.e. spends
> > more time on thread start-up/tear-down than actual processing time
> > if there are less references. And also we use all threads or single
> > thread during reference processing which seems less flexible on
> > thread counts. This patch calculates the worker counts from
> > dividing reference count by ReferencesPerThread(newly added
> > experimental option). 
> > My suggestion for the default value of ReferencePerThread is 1000
> > as it showed good results from some benchmarks. 
> > 
> > Notes:
> > 1. CMS ParNew is excluded from this patch because:
> >     a) There is a separate CR for CMS (JDK-6938732).
> >     b) It is tricky to manage switching single <-> MT processing
> > inside of ReferenceProcessor class for ParNew. Tony explained quite
> > well about the reason here ( https://bugs.openjdk.java.net/browse/J
> > DK-
> > 6938732?focusedCommentId=13932462&page=com.atlassian.jira.plugin.sy
> > stem.issuetabpanels:comment-tabpanel#comment-13932462 ). 
> >     c) CMS will be obsoleted in the future so not motivated to fix
> > within this patch.
> > 2. JDK-8203951 is the CR for removing temporarily added
> > flag(ReferenceProcessor::_has_adjustable_queue from webrev.0) to
> > manage ParNew. So the flag should be removed when CMS is obsoleted.
> > 3. Current logic of dividing by ReferencesPerThread would be
> > replaced with better implementation. e.g. time measuring
> > implementation etc. But I think current approach is simply and good
> > enough.
> > 4. This patch is based on JDK-8204094 and JDK-8204095, both are not
> > yet pushed so far.
> > 
> > CR: https://bugs.openjdk.java.net/browse/JDK-8043575
> > Webrev: http://cr.openjdk.java.net/~sangheki/8043575/webrev.0/
> > Testing: hs-tier 1~5 with/without ParallelRefProcEnabled
> > 
> > Thanks,
> > Sangheon
>