RFR(M) 8043575: Dynamically parallelize reference processing work

Sat Jun 9 06:13:42 UTC 2018

Hi Thomas,

On 6/8/18 7:52 AM, Thomas Schatzl wrote:
> Hi Sangheon,
>
> On Thu, 2018-06-07 at 10:50 -0700, sangheon.kim at oracle.com wrote:
>> Hi Thomas,
>>
>> On 6/7/18 7:12 AM, Thomas Schatzl wrote:
>>> Hi Sangheon,
>>>
>>>> [...]
>>>> As I noted in pre-review email thread, it showed better results
>>>> if I avoid using more than active cpus.
>>>> This is a kind of enhancement for better performance, so even
>>>> though the command-line option was set to use more than cpus(via
>>>> ParalelGCThreads), I think it is good to limit. This is different
>>>> from command-line option processing which lets users decide even
>>>> though the given options would results in worse performance.
>>>>
>>>> Or if you are saying I should use "active_processor_count()"? If
>>>> it is the case, I updated to use 'active_processor_count()'
>>>> instead of 'initial_active_processor_count()'.
>>>>
>>>> I would prefer to have this unless there's any reason of not
>>>> having it.
>>> First, always use active_processor_count() for current thread
>>> decisions. Initial_active_processor_count() is only here to get a
>>> consistent value of the processor count during initialization.
>>>
>>> What has been your test to determine this? GCOld?
>> Micro-benchmark which can create 1~3digit K of references. (mostly
>> 8k, 32k, 128k) Specjvm2008.Derby as it generates maximum of 12k
>> references.
>>
>> GCOld is my favorite to play with but it doesn't stress much
>> references.
> I will look into this some more.
Okay, thanks.

>
>>> I think this may be outdated as with recent changes scalability
>>> (with G1) should have improved a lot.
>> I tested before your rebuildRset patch, so a bit old.
>> But I'm not sure how recent changes would affect to this case. i.e.
>> if users sets quite big number than actual cpus, and then use those
>> threads on ref.proc. etc.. This patch is suggesting not to rely on
>> current setting in worst case.
>>
>> But I will not argue here, as you will the person to push! :)
> There have been improvements to actual reference processing in G1 that
> improved throughput for reference processing significantly.
Yes, I'm aware of these improvements.
Probably I'm too paranoid, but what I'm unsure is that if a user sets 
too large number of ParallelGCThreads, and G1 used a lot of threads than 
actually the host has, we would have less performance than limiting 
number of cpus.
Probably we would need follow-up CR to check if you want. :)

>
> [...]
>>> There is a WIP webrev at
>>>
>>> http://cr.openjdk.java.net/~tschatzl/8043575/webrev.3/
>>>
>>> for the curious (I think accomodated the review comments so far
>>> too).
>>> Still needs at least some more testing.
>>>
>>> And I need to remove the parallel gc support.
>> Okay.
>>
>>> I am also not sure whether there has been a decision on how this
>>> feature should be enabled, i.e. which switches need to be active to
>>> use it.
>>> I made it so that if ParallelRefProcEnabled is true, and
>>> ReferencesPerThread > 0, we enable the dynamic number of reference
>>> processing threads.
>>> I.e. if ParallelRefProcEnabled is false, dynamic number of
>>> reference processing threads is also turned off (because the user
>>> requested to not do parallel ref processing at all).
>> That is how works on my patch, too.
>> I should mention on my review request, thank you for bringing up
>> this topic!!
> I filed a preliminary CSR at https://bugs.openjdk.java.net/browse/JDK-8
> 204612 for this change.
I can review it when you are ready and before my vacation. Looks good to 
me as is though.

>
> I.e.
> - make ParallelRefProcEnabled with dynamic selection of thread numbers
> the default for G1.
> - (I do not think we need the CSR for the ReferencesPerThread flag, but
> I did anyway).
Okay, changing the default for G1 sounds a good idea.

>
>> Previously we discussed for changing ParallelRefProcEnabled option
>> to on/off/auto(enabling ReferencesPerThread). Of course, the name
>> also  should be changed too. It would be good to discuss under
>> separate CR.
>>
>> thomas.webrev.3 looks good to me.
> I wasn't expecting a review already :)
:-)

>
>> Some minor comments:
>> --------------------------
>> - I guess you didn't merge TestPrintReferences.java yet? So
>> intentionally not included the file on webrev.3?
> I had to fix it a bit for the latest version of webrev.3, which I think
> is ready for review.
>
>> --------------------------
>> src/hotspot/share/gc/shared/referenceProcessor.hpp
>> 215   bool        _adjust_no_of_processing_threads;
>>
>> 556   // True, to allow ergonomically changing a number of
>> processing
>> threads based on references.
>> 557   bool _adjust_no_of_processing_threads;
>> - There are 2 declaration of  _adjust_no_of_processing_threads. The
>> latter one exists on other class.
>>
>> --------------------------
>> (pre-existing from my webrev)
>> src/hotspot/share/gc/cms/concurrentMarkSweepGeneration.cpp
>> 5128   assert(workers->active_workers() == ergo_workers,
>> 5129          "Ergonomically chosen workers(%u) should be less than
>> or
>> equal to active workers(%u)",
>> 5130          ergo_workers, workers->active_workers());
>> - We can remove the assert or change the statement to remove 'less
>> than or'.
>>
>> --------------------------
>> src/hotspot/share/gc/cms/parNewGeneration.cpp
>> 797   assert(workers->active_workers() == ergo_workers,
>> 798          "Ergonomically chosen workers(%u) should be less than
>> or
>> equal to active workers(%u)",
>> 799          ergo_workers, workers->active_workers());
>> - Same as above.
>>
>> --------------------------
>> - Same for pcTasks.cpp and psScavenge.cpp too if we decide not to
>> include parallel gc in this patch.
>>
>> --------------------------
>> src/hotspot/share/gc/shared/referenceProcessor.cpp
>>    786   RefProcMTDegreeAdjuster a(this,
>>    787                             RefPhase1,
>>    788                             num_soft_refs);
>> - We can put at the same line. Same for other cases.
>>
>> Again thank you for taking this CR, Thomas!
> I think I fixed all this.
>
> Webrev is at http://cr.openjdk.java.net/~tschatzl/8043575/webrev.3/ .
>
> This webrev is based on top of latest jdk/jdk and https://bugs.openjdk.
> java.net/browse/JDK-8202845 .
>
> If you want to test parallel gc, you also need the fixes for JDK-
> 8204617 and JDK-8204618 currently out for review.
>
> Testing:
> hs-tier1-4,jdk-tier1-3 with +/-ParallelRefProcEnabled
Webrev.3 looks good to me.

Thanks,
Sangheon

>
> Thanks,
>    Thomas
>