<html><head></head><body>I think it returns either NULL or passes through LRB, I think this is fine and does not leak from-space-refs.<br><br>Roman<br><br><br><div class="gmail_quote">Am 11. Juni 2019 11:23:12 MESZ schrieb Aleksey Shipilev <shade@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">On 6/11/19 1:14 AM, Zhengyu Gu wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">On 6/10/19 4:32 AM, Aleksey Shipilev wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">On 6/10/19 2:06 AM, Zhengyu Gu wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;"> Please review this enhancement of native access barrier, 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: <a href="http://cr.openjdk.java.net/~zgu/JDK-8225483/webrev.00/">http://cr.openjdk.java.net/~zgu/JDK-8225483/webrev.00/</a><br></blockquote><br> *) It looks to me the webrev itself and the patch in it disagree. For example, webrev says the<br> entire oop_store_not_in_heap is #ifdef ASSERT-ed, but the patch has the ASSERT block internally. The<br> patch also has the unrelated change in shenandoahForwarding.hpp.<br></blockquote><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></blockquote><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></pre></blockquote></div><br>-- <br>Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.</body></html>