<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>