<div dir="ltr">Well the question that is underlying this thread is: does someone that uses DirectByteBuffer needs to ensure their liveness beyond method calls? <div><br></div><div class="gmail_extra"><div class="gmail_quote">It seems to be the case. <br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.8px">My take is that current DBB doesn't work. It's just "harder" to hit its bugs because the racing-point safepoint is often (unintentionally but luckily) eliminated, and the race needed to hit the safepoint when it does remain in compiled code is dynamically rare.</span></blockquote></div></blockquote><div><br></div><div>I tend to agree with Gil, and the example code that I have attached does seem to show that the compiler does not have any specific optimization fences when encountering Unsafe (or I would like to be explained what is going on, and would appreciate pointers to where in the hotspot source code those things are enforced). And this whole discussion shows that even if it were the case, relying on it might break at some point. <br></div><div><br></div><div>So as a Java programmer, I will make sure that when using direct buffers they stay reachable long enough.</div><div><br></div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Agreed that it's broken and doesn't work *by design*.</div><div><br></div><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote">get(int i) {<br>   long addr = ix(checkIndex(i));<br>   // There is a safepoint here which may or may not be removed depending on<br>   // unrelated compiler decisions.<br>   // "this" may no longer be reachable here.<br>   // If this is the last access to this buffer, and it goes out of scope "after" this get(),<br>   // GC can trigger here, enqueue the phantom ref, and the cleaner can fully<br>   // execute and free the associated off-heap memory at addr before the next<br>   // line below is executed...<br>   return ((unsafe.getByte(addr)))); // <—— Boom!<br>}</blockquote><div><br></div><div>Yes.  I'm not convinced that compiler isn't treating Unsafe operations as an optimization barrier, precluding live range of `this` being shortened.  Is there even a single reported/observed instance of DBB segfaulting or reading free'd memory in this case? This class and methods are very popular, and granted most of the time you don't instantiate a DBB that immediately goes out of scope, I'd expect this to occur out in the wild and ensuing bug reports.</div><div><br></div><div><blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" class="gmail_quote">public ByteBuffer put(int i, byte x) {<br>   long addr = ix(checkIndex(i));<br>   // There is a safepoint here which may or may not be removed depending on<br>   // unrelated compiler decisions.<br>   // "this" may no longer be reachable here, e.g. if method is inlined.<br>   // If this is the last put to this buffer, and it goes out of scope "after" this put(),<br>   // GC can trigger here, enqueue the phantom ref, and the cleaner can fully<br>   // execute and free the associated off-heap memory at addr before the next<br>   // line below is executed...<br>   unsafe.putByte(ix(addr, ((x)));  // <— Boom!<br>   return this; // This may not be used by inlining caller, and removed as dead code<br>}</blockquote></div><div> </div></div><div>Likewise here, I'm not sure compiler reasons about Unsafe operations so aggressively.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.8px">public ByteBuffer put(int i, byte x) {<br></span><span style="font-size:12.8px">   try {<br></span><span style="font-size:12.8px">       unsafe.putByte(ix(</span><span style="font-size:12.8px">checkIndex(i)), ((x)));<br></span><span style="font-size:12.8px">       return this;<br></span><span style="font-size:12.8px">   } finally {<br></span><span style="font-size:12.8px">       Fences.</span><span style="font-size:12.8px">reachabilityFence(this);<br></span><span style="font-size:12.8px">   }<br></span><span style="font-size:12.8px">}</span></blockquote><div><br></div><div>Yes.  This also reminds me that default inlining size needs to be reconsidered in light of changes like the above.  Putting try/finally {reachabilityFence(this)} adds about 14 bytes of bytecode alone, which may make some methods go over the default MaxInlineSize threshold.</div><div> </div></div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Dec 8, 2015 at 12:01 PM, Gil Tene <span dir="ltr"><<a href="mailto:gil@azul.com" target="_blank">gil@azul.com</a>></span> wrote:<br></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><span class=""><span><blockquote type="cite">The thread started with Alexandre<br>asking a question for which he then added a segfaulting example; we</blockquote><blockquote type="cite">tried to figure out why his didn't work and DBB works.</blockquote><div><br></div></span></span><div>My take is that current DBB doesn't work. It's just "harder" to hit its bugs because the racing-point safepoint is often (unintentionally but luckily) eliminated, and the race needed to hit the safepoint when it does remain in compiled code is dynamically rare. To highlight by analysis, rather than with a reproducer, where current DBB is vulnerable, look at the simplest forms of get() and put(). Since both get() and put() will tend to [hopefully] get inlined, the safepoint positions I highlight below can easily remain real even after optimization. Many current cleaner-related (and other phantom/weak/soft ref queueing handlers that release resources) have similar bugs, and Fences.reachabilityFence can finally be used to fix them.</div><div><br></div><div>get(int i) {<br>   return ((unsafe.getByte(ix(checkIndex(i))))); // <—— bug!<br>}<br><br>public ByteBuffer put(int i, byte x) {<br>   unsafe.putByte(ix(checkIndex(i)), ((x)));  // <—— bug!<br>   return this;<br>}<br><br>To highlight the bugs, let me split these up into steps:<br><br>get(int i) {<br>   long addr = ix(checkIndex(i));</div><div>   // There is a safepoint here which may or may not be removed depending on</div><div>   // unrelated compiler decisions.<br>   // "this" may no longer be reachable here.<br>   // If this is the last access to this buffer, and it goes out of scope "after" this get(),<br>   // GC can trigger here, enqueue the phantom ref, and the cleaner can fully<br>   // execute and free the associated off-heap memory at addr before the next</div><div>   // line below is executed...<br>   return ((unsafe.getByte(addr)))); // <—— Boom!<br>}<br><br>public ByteBuffer put(int i, byte x) {<br>   long addr = ix(checkIndex(i));<br><div>   // There is a safepoint here which may or may not be removed depending on</div>   // unrelated compiler decisions.</div><div>   // "this" may no longer be reachable here, e.g. if method is inlined.<br>   // If this is the last put to this buffer, and it goes out of scope "after" this put(),<br>   // GC can trigger here, enqueue the phantom ref, and the cleaner can fully<br>   // execute and free the associated off-heap memory at addr before the next</div><div>   // line below is executed...<br>   unsafe.putByte(ix(addr, ((x)));  // <— Boom!<br>   return this; // This may not be used by inlining caller, and removed as dead code<br>}<br><br><br>With reachabilityFence, this bug can finally be fixed with e.g.:<br><br>public ByteBuffer put(int i, byte x) {<br>   try {<br>       unsafe.putByte(ix(checkIndex(i)), ((x)));<br>       return this;<br>   } finally {<br>       Fences.reachabilityFence(this);<br>   }<br>}<br><br>get(int i) {<br>   try {<br>       return ((unsafe.getByte(ix(checkIndex(i)))));<br>   } finally {<br>       Fences.reachabilityFence(this);<br>   }<br>}<br></div><div><div class="h5"><div><div><br><div><blockquote type="cite"><div>On Dec 8, 2015, at 7:38 AM, Vitaly Davidovich <<a href="mailto:vitalyd@gmail.com" target="_blank">vitalyd@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-size:12.8px">If you're talking about simply observing the effects of an object being collected while method invocations on that object are still in flight, see this article:</span><br style="font-size:12.8px"><a href="http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms" rel="noreferrer" style="font-size:12.8px" target="_blank">http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms</a><br style="font-size:12.8px"><span style="font-size:12.8px">We have run into this issue numerous times in various situations, which is why we're happy to see reachabilityFence() come into being.  So yes, it's already broken.</span></blockquote><div><br></div><div>Nope, I'm talking about breaking existing DBB code as it stands today.  Alexandre already posted an example of where similar but subtly different (with respect to DBB) code breaks, so I'm well aware it's a problem.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 8, 2015 at 10:22 AM, David M. Lloyd <span dir="ltr"><<a href="mailto:david.lloyd@redhat.com" target="_blank">david.lloyd@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On 12/08/2015 08:40 AM, Vitaly Davidovich wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
    The lifetime, natural or otherwise, of an instance does not survive<br>
    until an instance method returns because, a lot of the time, that<br>
    instance method is inlined.<br>
<br>
<br>
You're talking about optimization here (inlining); by "natural" I meant<br>
the naive/no optimization case (e.g. interpreter, debugger attached<br>
w/breakpoint in method, etc).<br>
<br>
    It's not just HotSpot, though: some VMs are even more aggressive, and<br>
<br>
Which java VMs are these? Just curious.<br>
<br>
    we have seen finalizers executed even before constructors have<br>
    completed.  And that is allowed by the specification.<br>
<br>
<br>
Ok, but that's beside the point, really.  Surely if compiler can<br>
optimize and arrange for liveness to allow for it, then it's a good<br>
thing it does that.  My point isn't that this cannot happen due to spec,<br>
but rather that in places like DBB where `this` is used after the Unsafe<br>
call the  compiler has to schedule things differently in order to reduce<br>
lifetime.  And my point is that compilers generally tend to be cautious<br>
in doing things that may break code.  This is the practical aspect we<br>
were referring to - it's actual humans writing these optimizations, and<br>
they're sensitive to breaking code, particularly in java.<br>
Theoretically, yes, anything is possible.<br>
<br>
    It's already broken.<br>
<br>
<br>
Sure.  Now try to submit a patch to Hotspot that will break this case,<br>
even if allowed by spec, and see how far you get :).<br>
</blockquote>
<br></span>
If you're talking about simply observing the effects of an object being collected while method invocations on that object are still in flight, see this article:<br>
<br>
<a href="http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms" rel="noreferrer" target="_blank">http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms</a><br>
<br>
We have run into this issue numerous times in various situations, which is why we're happy to see reachabilityFence() come into being.  So yes, it's already broken.<span><font color="#888888"><br>
-- <br>
- DML</font></span><div><div><br>
_______________________________________________<br>
Concurrency-interest mailing list<br>
<a href="mailto:Concurrency-interest@cs.oswego.edu" target="_blank">Concurrency-interest@cs.oswego.edu</a><br>
<a href="http://cs.oswego.edu/mailman/listinfo/concurrency-interest" rel="noreferrer" target="_blank">http://cs.oswego.edu/mailman/listinfo/concurrency-interest</a><br>
</div></div></blockquote></div><br></div>
_______________________________________________<br>Concurrency-interest mailing list<br><a href="mailto:Concurrency-interest@cs.oswego.edu" target="_blank">Concurrency-interest@cs.oswego.edu</a><br><a href="http://cs.oswego.edu/mailman/listinfo/concurrency-interest" target="_blank">http://cs.oswego.edu/mailman/listinfo/concurrency-interest</a><br></div></blockquote></div><br></div></div></div></div></div></blockquote></div><br></div>
<br>_______________________________________________<br>
Concurrency-interest mailing list<br>
<a href="mailto:Concurrency-interest@cs.oswego.edu">Concurrency-interest@cs.oswego.edu</a><br>
<a href="http://cs.oswego.edu/mailman/listinfo/concurrency-interest" rel="noreferrer" target="_blank">http://cs.oswego.edu/mailman/listinfo/concurrency-interest</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div>Alexandre</div></div></div>
</div></div>