RFR(XS): JDK-8212122: Allow ReferenceProcessor to always be MT processing

[Roman Kennke]

Am 13.10.18 um 11:38 schrieb Roman Kennke:
> 
>>> Currently, in ReferenceProcessor, the mt_processing flag is adjusted for
>>> each phase depending on the number of workers: if it's 1 worker, then
>>> mt_processing is disabled, for more than 1 worker, mt_processing is
>>> enabled. Depending on the flag, ref-processing is either done through
>>> AbstractRefProcTaskExecutor, or directly in VMThread. However, we have a
>>> situation where we always want to execute via the executor, and never
>>> directly. Instead we decide in the executor whether or not we want to
>>> execute directly in the VMThread.
>>>
>>> A simple change allows a subclass of ReferenceProcessor to change this
>>> behavior.
>>>
>>> In case you're interested why we need to do this: Shenandoah requires
>>> (for each worker) a scope around reference processing. In current
>>> scheme, we'd need to put one scope around
>>> RP::process_discovered_reference() to cover the 1-threaded case, and one
>>> scope inside each worker to cover those. However, we cannot handle this
>>> sort of weirdly nested scoping.
>>>
>>> (If it was me, I'd rework this whole execution scheme to always call the
>>> MT config, and let RP figure out and execute directly in VMThread if
>>> it's only 1 worker, and not put this burden on the caller to set up both
>>> for 1-threaded and N-threaded execution. This is just an ugly mess.)
>> I agree with you.
>> In addition to Kim's comment which covered ParallelGC case,
>> 1) For CMS, single vs. MT processing should be decided before calling
>> RP  because of CMS specific implementation. For more detail, please
>> refer comments at https://bugs.openjdk.java.net/browse/JDK-6938732
>> 2) For G1, MT processing is set to true by default. But still caller has
>> 2 paths as you described. I'm not sure whether single thread case is
>> still needed there or not because RP is deciding to use single or
>> multiple threads. Probably we can remove it unless an user really wants
>> to force using single thread only. :)
> 
> 
> The problem is the (rather new) ergonomics code in RP that scales number
> of threads to spin up based on the amount of work to do. And if it ends
> up being only 1 thread, then it goes down the single-threaded-path.
> Otherwise I wouldn't actually need this change... (that might be another
> approach to this problem: don't actually change _mt_processing flag at
> all, and call the executor even with 1 thread. In Shenandoah, we do this
> and it works fine, and we special-case the 1-thread-path by not spinning
> up the workers and execute it in the calling thread (which is the VMThread).

In other words, this is a fix that actually removes 'cruft', and seems
less surprising API-wise: i.e. if caller asks for mt_processing, then
don't change it, and leave it to caller to execute in VMThread if it wishes.

Updated webrev:
http://cr.openjdk.java.net/~rkennke/JDK-8212122/webrev.01/

An example how G1 can be made to strictly do the current behaviour (i.e.
execute in VMThread when ergo_workers==1):

http://cr.openjdk.java.net/~rkennke/JDK-8212122/webrev.01.g1/

... altough I haven't tested it at all. It works well with Shenandoah,
but I don't know if you'd actually do it/want it/ask me to do it ;-)

What do you think? Which approach do you like best (except the huge
parallel/CMS/WorkGang/ReferenceProcessor reworking)?

Thanks,
Roman

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181015/538655d7/signature.asc>