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