RFR 8232747: Shenandoah: Concurrent GC should deactivate SATB before processing weak roots

[Zhengyu Gu]

Hi Aleksey,

On 10/22/19 9:55 AM, Aleksey Shipilev wrote:
> On 10/22/19 3:38 PM, Zhengyu Gu wrote:
>> This is the counterpart of JDK-8231999[1] for Shenandoah concurrent GC. Shenandoah needs to
>> deactivate SATB barrier before processing weak roots, to avoid barrier side-effects on its paths.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8232747
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8232747/webrev.00/index.html
> 
> *) Mmm... In ShenandoahConcurrentMark::finish_mark_from_roots, there is a call:
>    _heap->parallel_cleaning(full_gc);

It is removed by following.

diff --git 
a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp 
b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp
--- a/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp
+++ b/src/hotspot/share/gc/shenandoah/shenandoahConcurrentMark.cpp
@@ -442,8 +442,6 @@
      weak_refs_work(full_gc);
    }

-  _heap->parallel_cleaning(full_gc);
-
    assert(task_queues()->is_empty(), "Should be empty");
    TASKQUEUE_STATS_ONLY(task_queues()->print_taskqueue_stats());
    TASKQUEUE_STATS_ONLY(task_queues()->reset_taskqueue_stats());

> 
>    Does it mean new code would perform cleaning twice?
> 
> *) This comment relates to keeping has_forwarded_objects set on cancelled path:
> 
>     // If we needed to update refs, and concurrent marking has been cancelled,
>     // we need to finish updating references.
> 
> ...current placement loses that connection. Suggestion:
> 
>     // If this cycle was updating references and got cancelled, we need to keep
>     // the flag on, for subsequent phases to deal with it.
> 
> *) Maybe we should inline stop_concurrent_marking everywhere to make the flow more obvious...
> 

Updated: http://cr.openjdk.java.net/~zgu/JDK-8232747/webrev.01/index.html

Thanks,

-Zhengyu