RFR 8225483: Shenandoah: Enhance native access barrier

[Roman Kennke]

OK by me.
Thanks!
 Roman

Am 11. Juni 2019 16:25:21 MESZ schrieb Zhengyu Gu <zgu at redhat.com>:
>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.

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20190611/98e1261a/attachment.htm>