RFR(S) 8225813: Shenandoah: Obsolete ShenandoahSuspendibleWorkers flag, as suspendible workers are required

[Zhengyu Gu]

On 6/16/19 10:21 AM, Zhengyu Gu wrote:
> 
> 
> On 6/16/19 7:27 AM, Roman Kennke wrote:
>> Hi Zhengyu,
>>
>> So as far as I can see, we are looking at 2 potential issues:
>>
>> - Non-GC safepoint work might use our GC workers. This would only
>> possibly be heap-iteration, and as far as I remember, heap-iteration is
>> single-threaded and uses the VMThread. Besides: if this would happen,
>> we'd already have the problem. I don't think this is the case.
>> (Safepoint cleanup has this problem, and we use a separate thread pool
>> for precisely this reason.)
> 
> Right. It is single-threaded.
> 
>>
>> - Non-GC safepoint work needs to look at GC roots, and concurrent
>> processing of GC roots might conflict with that.
>>
>> As far as I see it, this problem could also be solved by employing the
>> correct barriers. I believe this only affects heap-iteration. So when
>> heap-iteration scans the GC roots (during conc-evac), it needs to do the
>> LRB to avoid exposing from-space-objects in the iteration. Later on,
>> when we also want to do concurrent cleanup, it also needs to filter
>> unreachable objects by not seeding the iteration with objects that are
>> unreachable (as per the marking bitmap, not the aux bitmap!).
>>
> Our current solution is to use the same roots for heap iteration, as 
> current GC cycle, so both see the same live object graph.
> 
> At this phase, the problems I saw, I think, can be solved with 
> additional cares to avoid suspending workers, e.g. not clear claimed 
> flag for CLDG and using _claim_none to for heap iteration, etc. but I 
> don't think it worth the efforts.

Second thoughts, maybe worth for releases that can not do concurrent 
class unloading, e.g. pre-JDK13.

-Zhengyu

> 
> This change is inavoidable for concurrent class unloading and concurrent 
> code root processing:
> 
> - SystemDictionary::do_unloading(), by design, can not run through 
> safepoints (JDK-8222223),
> 
> - Some CLDG related VM operations, such as ClassLoaderStatsOperation and
> ClassLoaderHierarchyOperation are no longer safe, if class unloading run 
> through them.
> 
> - I suspect some IC related also no longer safe.
> 
> -Zhengyu
> 
> 
> 
> 
>> I might be missing something there?
>>
>> The reason why I'm looking at avoiding stopping GC threads at safepoints
>> is that it may introduce additional latencies when bringing Java threads
>> to (non-GC) safepoints like biased locking or deopt, by having them wait
>> for GC work (especially when we have non-interruptible not-quite-short
>> GC work like GC roots processing).
>>
>> Roman
>>
>>> On 6/15/19 9:26 PM, Zhengyu Gu wrote:
>>>> On 6/15/19 3:35 PM, Roman Kennke wrote:
>>>>> Can you give a little bit of explanation why workers need to be
>>>>> suspended at safepoints for concurrent root processing?
>>>>
>>>> Concurrent processing uses workers and safepoint parallel task also
>>>> utilizes workers, and shenandoah shares workers, so they can not 
>>>> overlap.
>>>
>>> Should be more articulate. I don't mean GC safepoint parallel tasks,
>>> which can not overlap. But the tasks, such as, heap iteration that might
>>> utilize workers.
>>>
>>> Also, heap iteration walks CLDG at a safepoint, that may interfere CLDG
>>> walking during concurrent CLDG evacuation.
>>>
>>> -Zhengyu
>>>
>>>>
>>>>
>>>> -Zhengyu
>>>>
>>>>>
>>>>> Other than that, the patch looks good.
>>>>>
>>>>> Roman
>>>>>
>>>>>> Please review this patch that makes suspendible workers required,
>>>>>> instead of optional, and obsoletes the corresponding flag.
>>>>>>
>>>>>> Suspendible workers are required for concurrent root processing.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225813
>>>>>> Webrev:
>>>>>> http://cr.openjdk.java.net/~zgu/JDK-8225813/webrev.00/index.html
>>>>>>
>>>>>> Test:
>>>>>>     hotspot_gc_shenandoah (fastdebug and release)
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>
>>