RFR 8225483: Shenandoah: Enhance native access barrier

[Zhengyu Gu]

Updated: http://cr.openjdk.java.net/~zgu/JDK-8225483/webrev.02/

Okay now?

Thanks,

-Zhengyu

On 6/11/19 6:43 AM, Roman Kennke wrote:
> I think it returns either NULL or passes through LRB, I think this is 
> fine and does not leak from-space-refs.
> 
> Roman
> 
> 
> Am 11. Juni 2019 11:23:12 MESZ schrieb Aleksey Shipilev <shade at redhat.com>:
> 
>     On 6/11/19 1:14 AM, Zhengyu Gu wrote:
> 
>         On 6/10/19 4:32 AM, Aleksey Shipilev wrote:
> 
>             On 6/10/19 2:06 AM, Zhengyu Gu wrote:
> 
>                 Please review this enhancement of native access barrier,
>                 in preparation for concurrent root
>                 processing.
> 
>                 Bug: https://bugs.openjdk.java.net/browse/JDK-8225483
>                 Webrev:
>                 http://cr.openjdk.java.net/~zgu/JDK-8225483/webrev.00/
> 
> 
>             *) It looks to me the webrev itself and the patch in it
>             disagree. For example, webrev says the
>             entire oop_store_not_in_heap is #ifdef ASSERT-ed, but the
>             patch has the ASSERT block internally. The
>             patch also has the unrelated change in shenandoahForwarding.hpp.
> 
> 
>         Sorry, it is a broken webrev.
> 
>         Updated: http://cr.openjdk.java.net/~zgu/JDK-8225483/webrev.01/
> 
> 
>     This patch cannot go to jdk/jdk. It can only go to sh/jdk.
> 
>     *) This block:
> 
>        99   ShenandoahHeap* const heap = ShenandoahHeap::heap();
>       100   shenandoah_assert_marked_if(NULL, value, !CompressedOops::is_null(value) &&
>     heap->is_evacuation_in_progress());
> 
>     Can be just:
> 
>         shenandoah_assert_marked_if(NULL, value, !CompressedOops::is_null(value) &&
>     ShenandoahHeap::heap()->is_evacuation_in_progress());
> 
>     *) After the change above is done, you can make SBS::oop_store_not_in_heap available always: e.g.
>     drop the ASSERT around it.
> 
>     *) I think the block in oop_load_not_in_heap needs the comment why this is done. This is a weird
>     exception anyway: it leaks the from-space pointers from the weak roots, no? What actually guarantees
>     we would not store that from-space pointer somewhere *on heap*?
> 
>     *) This block can be simplified:
> 
>         ShenandoahHeap* const heap = ShenandoahHeap::heap();
>         if (heap->is_evacuation_in_progress()) {
>           ShenandoahMarkingContext* const marking_context = heap->complete_marking_context();
>           if (!marking_context->is_marked(value)) {
>             return NULL;
>           }
>         }
> 
>     to:
> 
>         ShenandoahHeap* const heap = ShenandoahHeap::heap();
>         if (heap->is_evacuation_in_progress() && !heap->complete_marking_context()->is_marked(value)) {
>           return NULL;
>         }
> 
>     -Aleksey
> 
> 
> -- 
> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.