[concurrency-interest] DirectByteBuffers and reachabilityFence

Vitaly Davidovich vitalyd at gmail.com
Wed Dec 9 11:46:27 EST 2015


sent from my phone
On Dec 9, 2015 11:34 AM, "Gil Tene" <gil at azul.com> wrote:
>
>
>
> Sent from Gil's iPhone
>
> On Dec 9, 2015, at 8:16 AM, Vitaly Davidovich <vitalyd at gmail.com> wrote:
>
>>> an implicit implication that 'this' must remain reachable until after
the last instruction in each of its instance methods
>>
>>
>> Why wouldn't this work, at least as a compromise?
>
>
> Didn't say it wouldn't work. I said it >doesn't do it today.

Is this a fact that it doesn't do it?

>
>> If there's unsafe.foo() inside a method, extend `this` lifetime to
beyond that unsafe call.  Or if there's a use of `this` after unsafe.foo(),
do not shorten the lifetime of `this` to prior to unsafe.foo(), even if the
operations are otherwise schedulable like that.
>
>
> I wouldn't limit this to unsafe. I would just do it for all instance
method. Period. The bugs that have to do with loss of reachability to this
exist even without uses of unsafe. Unsafe use cases (like DBB) are just a
useful ways to demonstrate how bad things are (a SEGV catches people's
attention more than other semantic wrongness).
>

I'd have an issue with extending it blindly.  What if you have a long
running ordinary java method that doesn't use 'this' for majority of the
execution time?

You only have to do this when compiler is too smart and sees all accesses
because it fully inlined.  Non-inlined methods are black boxes and compiler
cannot reason across them (afterall, that's how current reachabilityFence
is implemented).

> To be specific: the same semantic premature loss of reachability issue
exists around any resource-release-on-loss-of-reachability logic in Java
today. Regardless of their choice of finalizers, cleaners, phantom refs, or
weak or soft refs. And regales of whether the resource involved is
"physical" (like memory) or logical (like a key or an id).  E.g. Files can
be prematurely closed, their fds recycled, and their last I/O operations
done to the wrong file or channel.  Logical window Ids can be prematurely
recycled and their last graphic operations done to the wrong window or to
bad memory. Etc. etc. etc.
>
> I'm not saying that each of these bugs actually exists right now (I
haven't specifically hunted them down to check), but I *am* saying that the
typical mechanisms each of them would use to do
resource-release-on-loss-of-reachability (whether in the JDK or in user
code) is inherently flawed under the current JLS and JVM specs, and that
much like DBB's innocently written bugs, I expect to find these in many
places that I look...
>
>>
>> On Wed, Dec 9, 2015 at 10:57 AM, Gil Tene <gil at azul.com> wrote:
>>>
>>> Just like in Justin's example, DBB has multiple methods that don't use
'this' after their call to unsafe. Most of the get() and put() variants
behave like that in DBB, and they are all exposed to a cleaner running and
freeing the buffer's backing store before the unsafe call is ever made.
>>>
>>> AFAICT there is nothing the compilers do to protect against this.
Specifically, there is nothing being done to extend the reachability of
'this' to beyond the point where an unsafe call is made in an instance
method. And doing anything about it for unsafe specifically would be
"hard". The unsafe call has no relation to 'this' except that it is being
called from within the instance method, and uses arguments that were at
some point in the (recent) past derived from 'this'. Without a reachability
fence, or an implicit implication that 'this' must remain reachable until
after the last instruction in each of its instance methods, there is
nothing you can do (IMO) to plug up this bug.
>>>
>>> Sent from Gil's iPhone
>>>
>>> On Dec 9, 2015, at 7:30 AM, Vitaly Davidovich <vitalyd at gmail.com> wrote:
>>>
>>>> Your example code differs from DBB in that you don't use `this` after
call to unsafe, as mentioned before, so the compiler fence (if any) is not
applicable.
>>>>
>>>> Maybe you should bring this up on the hotspot compiler dev mailing
list and get a definitive answer on how this is modeled today.
>>>>
>>>> sent from my phone
>>>>
>>>> On Dec 9, 2015 4:30 AM, "Alexandre De Champeaux" <adc at quartetfs.com>
wrote:
>>>>>
>>>>> Well the question that is underlying this thread is: does someone
that uses DirectByteBuffer needs to ensure their liveness beyond method
calls?
>>>>>
>>>>> It seems to be the case.
>>>>>>>
>>>>>>> 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.
>>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>> So as a Java programmer, I will make sure that when using direct
buffers they stay reachable long enough.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Agreed that it's broken and doesn't work *by design*.
>>>>>>
>>>>>>> get(int i) {
>>>>>>>    long addr = ix(checkIndex(i));
>>>>>>>    // There is a safepoint here which may or may not be removed
depending on
>>>>>>>    // unrelated compiler decisions.
>>>>>>>    // "this" may no longer be reachable here.
>>>>>>>    // If this is the last access to this buffer, and it goes out of
scope "after" this get(),
>>>>>>>    // GC can trigger here, enqueue the phantom ref, and the cleaner
can fully
>>>>>>>    // execute and free the associated off-heap memory at addr
before the next
>>>>>>>    // line below is executed...
>>>>>>>    return ((unsafe.getByte(addr)))); // <—— Boom!
>>>>>>> }
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>>> public ByteBuffer put(int i, byte x) {
>>>>>>>    long addr = ix(checkIndex(i));
>>>>>>>    // There is a safepoint here which may or may not be removed
depending on
>>>>>>>    // unrelated compiler decisions.
>>>>>>>    // "this" may no longer be reachable here, e.g. if method is
inlined.
>>>>>>>    // If this is the last put to this buffer, and it goes out of
scope "after" this put(),
>>>>>>>    // GC can trigger here, enqueue the phantom ref, and the cleaner
can fully
>>>>>>>    // execute and free the associated off-heap memory at addr
before the next
>>>>>>>    // line below is executed...
>>>>>>>    unsafe.putByte(ix(addr, ((x)));  // <— Boom!
>>>>>>>    return this; // This may not be used by inlining caller, and
removed as dead code
>>>>>>> }
>>>>>>
>>>>>>
>>>>>> Likewise here, I'm not sure compiler reasons about Unsafe operations
so aggressively.
>>>>>>
>>>>>>> public ByteBuffer put(int i, byte x) {
>>>>>>>    try {
>>>>>>>        unsafe.putByte(ix(checkIndex(i)), ((x)));
>>>>>>>        return this;
>>>>>>>    } finally {
>>>>>>>        Fences.reachabilityFence(this);
>>>>>>>    }
>>>>>>> }
>>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>>
>>>>>> On Tue, Dec 8, 2015 at 12:01 PM, Gil Tene <gil at azul.com> wrote:
>>>>>>>>
>>>>>>>> The thread started with Alexandre
>>>>>>>> asking a question for which he then added a segfaulting example; we
>>>>>>>>
>>>>>>>> tried to figure out why his didn't work and DBB works.
>>>>>>>
>>>>>>>
>>>>>>> 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.
>>>>>>>
>>>>>>> get(int i) {
>>>>>>>    return ((unsafe.getByte(ix(checkIndex(i))))); // <—— bug!
>>>>>>> }
>>>>>>>
>>>>>>> public ByteBuffer put(int i, byte x) {
>>>>>>>    unsafe.putByte(ix(checkIndex(i)), ((x)));  // <—— bug!
>>>>>>>    return this;
>>>>>>> }
>>>>>>>
>>>>>>> To highlight the bugs, let me split these up into steps:
>>>>>>>
>>>>>>> get(int i) {
>>>>>>>    long addr = ix(checkIndex(i));
>>>>>>>    // There is a safepoint here which may or may not be removed
depending on
>>>>>>>    // unrelated compiler decisions.
>>>>>>>    // "this" may no longer be reachable here.
>>>>>>>    // If this is the last access to this buffer, and it goes out of
scope "after" this get(),
>>>>>>>    // GC can trigger here, enqueue the phantom ref, and the cleaner
can fully
>>>>>>>    // execute and free the associated off-heap memory at addr
before the next
>>>>>>>    // line below is executed...
>>>>>>>    return ((unsafe.getByte(addr)))); // <—— Boom!
>>>>>>> }
>>>>>>>
>>>>>>> public ByteBuffer put(int i, byte x) {
>>>>>>>    long addr = ix(checkIndex(i));
>>>>>>>    // There is a safepoint here which may or may not be removed
depending on
>>>>>>>    // unrelated compiler decisions.
>>>>>>>    // "this" may no longer be reachable here, e.g. if method is
inlined.
>>>>>>>    // If this is the last put to this buffer, and it goes out of
scope "after" this put(),
>>>>>>>    // GC can trigger here, enqueue the phantom ref, and the cleaner
can fully
>>>>>>>    // execute and free the associated off-heap memory at addr
before the next
>>>>>>>    // line below is executed...
>>>>>>>    unsafe.putByte(ix(addr, ((x)));  // <— Boom!
>>>>>>>    return this; // This may not be used by inlining caller, and
removed as dead code
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> With reachabilityFence, this bug can finally be fixed with e.g.:
>>>>>>>
>>>>>>> public ByteBuffer put(int i, byte x) {
>>>>>>>    try {
>>>>>>>        unsafe.putByte(ix(checkIndex(i)), ((x)));
>>>>>>>        return this;
>>>>>>>    } finally {
>>>>>>>        Fences.reachabilityFence(this);
>>>>>>>    }
>>>>>>> }
>>>>>>>
>>>>>>> get(int i) {
>>>>>>>    try {
>>>>>>>        return ((unsafe.getByte(ix(checkIndex(i)))));
>>>>>>>    } finally {
>>>>>>>        Fences.reachabilityFence(this);
>>>>>>>    }
>>>>>>> }
>>>>>>>
>>>>>>>> On Dec 8, 2015, at 7:38 AM, Vitaly Davidovich <vitalyd at gmail.com>
wrote:
>>>>>>>>
>>>>>>>>> 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:
>>>>>>>>> http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> On Tue, Dec 8, 2015 at 10:22 AM, David M. Lloyd <
david.lloyd at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On 12/08/2015 08:40 AM, Vitaly Davidovich wrote:
>>>>>>>>>>
>>>>>>>>>>     The lifetime, natural or otherwise, of an instance does not
survive
>>>>>>>>>>     until an instance method returns because, a lot of the time,
that
>>>>>>>>>>     instance method is inlined.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> You're talking about optimization here (inlining); by "natural"
I meant
>>>>>>>>>> the naive/no optimization case (e.g. interpreter, debugger
attached
>>>>>>>>>> w/breakpoint in method, etc).
>>>>>>>>>>
>>>>>>>>>>     It's not just HotSpot, though: some VMs are even more
aggressive, and
>>>>>>>>>>
>>>>>>>>>> Which java VMs are these? Just curious.
>>>>>>>>>>
>>>>>>>>>>     we have seen finalizers executed even before constructors
have
>>>>>>>>>>     completed.  And that is allowed by the specification.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ok, but that's beside the point, really.  Surely if compiler can
>>>>>>>>>> optimize and arrange for liveness to allow for it, then it's a
good
>>>>>>>>>> thing it does that.  My point isn't that this cannot happen due
to spec,
>>>>>>>>>> but rather that in places like DBB where `this` is used after
the Unsafe
>>>>>>>>>> call the  compiler has to schedule things differently in order
to reduce
>>>>>>>>>> lifetime.  And my point is that compilers generally tend to be
cautious
>>>>>>>>>> in doing things that may break code.  This is the practical
aspect we
>>>>>>>>>> were referring to - it's actual humans writing these
optimizations, and
>>>>>>>>>> they're sensitive to breaking code, particularly in java.
>>>>>>>>>> Theoretically, yes, anything is possible.
>>>>>>>>>>
>>>>>>>>>>     It's already broken.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sure.  Now try to submit a patch to Hotspot that will break this
case,
>>>>>>>>>> even if allowed by spec, and see how far you get :).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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:
>>>>>>>>>
>>>>>>>>> http://www.infoq.com/articles/Fatal-Flaw-Finalizers-Phantoms
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>> --
>>>>>>>>> - DML
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Concurrency-interest mailing list
>>>>>>>>> Concurrency-interest at cs.oswego.edu
>>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Concurrency-interest mailing list
>>>>>>>> Concurrency-interest at cs.oswego.edu
>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Concurrency-interest mailing list
>>>>>> Concurrency-interest at cs.oswego.edu
>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alexandre
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://cs.oswego.edu/pipermail/concurrency-interest/attachments/20151209/ad4c64ee/attachment-0001.html>


More information about the Concurrency-interest mailing list