RFR(M) 8043575: Dynamically parallelize reference processing work

[sangheon.kim at oracle.com]

Hi Thomas,

Thank you for reviewing this.

On 6/4/18 1:56 AM, Thomas Schatzl wrote:
> 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.
Removed the comment.

>
> - g1FullGCReferenceProcessorExecutor.cpp: I think
> G1FullGCReferenceProcessingExecutor::run_task(AbstractGangTask*) is not
> used any more.
Right, removed it.

>
> - 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.
Right, should be same.

>
> - 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."
Updated as you suggested above.

>
> - would you mind renaming ReferenceProcessor::has_adjustable_queue? Not
> the queues are adjusted, but the number of processing threads.
How about ReferenceProcessor::has_adjustable_num_of_proc_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.
Changed to always check.
This is webrev.1 bug :) When I started working on this, that condition 
was fixed but not now.

>
> - 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?
Sure, as we discussed separately, I will reassign to you at some point.

Let me post webrev.2 after addressing Kim's comment.

Thanks,
Sangheon


>
> 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
>>