DirectByteBuffers and reachabilityFence

classic Classic list List threaded Threaded
148 messages Options
123456 ... 8
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
On 08/12/15 19:17, Vitaly Davidovich wrote:

>> [me:]
>> Absolutely, yes.  We already know that very similar code breaks.
>
> "Very similar" is slightly different though.
>
>> Sure there is: write a field (a counter, say) in your methods and read
>> it in the finalizer.  Make sure that you do something with the field
>> in the finalizer to ensure it's not eliminated: a volatile write will
>> do.  This is fully JLS-compliant; reachabilityFence() is just an
>> optimization.
>
> C'mon, really?? :)

I wonder how many times I'm going to wrote "Absolutely, yes."

> So now I'm going to write to dummy fields? And what are you going to
> do with it in a finalizer?

Update a global volatile.

> And of course, a Sufficiently Smart Compiler could detect
> (theoretically, of course :)) that this is all just dummy ops and
> remove them.

No, it can't.  Because the JLS says so.  IMVHO it'd be much better to
stop trying to guess what a compiler might do and simply write in the
language instead.

> In my opinion, the current lack of optimization (accidental or not)
> should be somehow encoded/made intentional.

I have in the past argued that methods of classes with finalizers
should automagically extend the lifetime of the "this" object.
However, I was on the losing side, and reachabilityFence() is the
compromise result.  That's okay, really: it solves the practical
problem.

> Perhaps treat Unsafe::anything() as a full compiler optimization
> fence, if it's not already.

That one really is a no-hoper.  The idea of NIO ByteBuffers is "as
fast as C" and full fences would be a pretty major regression.

Andrew.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by David Holmes-6
On 09/12/15 09:21, David Holmes wrote:

> The happens-before requirement was deliberately set the way it is to
> show that an object can not be finalized before its construction has
> completed (whether normally or abnormally). So I would agree with
> Justin that it should not be possible for an object to be finalized
> before construction has completed - regardless of potential compiler
> optimizations etc.

Hmmm, okay.  It might be that I misremembered or perhaps it was a bug,
but I don't think so.  Is there really a happens-before relationship
between a constructor of an object with no volatile or final fields
and its finalizer?

Andrew.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

David Holmes-6
Andrew Haley writes:

> On 09/12/15 09:21, David Holmes wrote:
>
> > The happens-before requirement was deliberately set the way it is to
> > show that an object can not be finalized before its construction has
> > completed (whether normally or abnormally). So I would agree with
> > Justin that it should not be possible for an object to be finalized
> > before construction has completed - regardless of potential compiler
> > optimizations etc.
>
> Hmmm, okay.  It might be that I misremembered or perhaps it was a bug,
> but I don't think so.  Is there really a happens-before relationship
> between a constructor of an object with no volatile or final fields
> and its finalizer?

My recollection - and I've been trying to find this in the JavaMemoryModel
archives - is that the happens-before between constructor and finalizer was
introduced because the normal "trick" of using synchronization was very
counter-intuitive in constructors given the object has not been published.

That said, A happens-before B simply establishes visibility/ordering rules
for the case when A occurs before B. It says nothing about B occurring
before A, or B and A overlapping. Other mechanisms have to force the
temporal order. So it may be I misspoke my support of Justin's position. :(

David
-----

 
> Andrew.
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Doug Lea
In reply to this post by Andrew Haley
On 12/09/2015 05:33 AM, Andrew Haley wrote:

>> In my opinion, the current lack of optimization (accidental or not)
>> should be somehow encoded/made intentional.
>
> I have in the past argued that methods of classes with finalizers
> should automagically extend the lifetime of the "this" object.
> However, I was on the losing side, and reachabilityFence() is the
> compromise result.  That's okay, really: it solves the practical
> problem.
>

I don't think anyone "lost" these arguments, but further action
was postponed at least in the context of JMM-related updates.

Among the options is to introduce a @Finalized annotation
for a field of an object. Front-end compilers would
then insert reachabilityFence() after each use. Not necessarily
immediately after though. Optimal placement is non-trivial, which
is one reason this wasn't widely agreed on. It would be great
if some IDE's tried out this kind of support inside IDEs though.

See discussions on jmm-dev list mostly in August 2014.
   http://mail.openjdk.java.net/pipermail/jmm-dev/

(Also, someday, the JLS probably should be updated to explicitly
mention impact of reachabilityFence. The wording change
would basically be the same as the method spec, but a few other
clarifications done at the same time might be helpful.

-Doug


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Andrew Haley

sent from my phone
On Dec 9, 2015 5:33 AM, "Andrew Haley" <[hidden email]> wrote:
>
> On 08/12/15 19:17, Vitaly Davidovich wrote:
> >> [me:]
> >> Absolutely, yes.  We already know that very similar code breaks.
> >
> > "Very similar" is slightly different though.
> >
> >> Sure there is: write a field (a counter, say) in your methods and read
> >> it in the finalizer.  Make sure that you do something with the field
> >> in the finalizer to ensure it's not eliminated: a volatile write will
> >> do.  This is fully JLS-compliant; reachabilityFence() is just an
> >> optimization.
> >
> > C'mon, really?? :)
>
> I wonder how many times I'm going to wrote "Absolutely, yes."

As many silly suggestions/workarounds as you propose :).

>
> > So now I'm going to write to dummy fields? And what are you going to
> > do with it in a finalizer?
>
> Update a global volatile.

And take perf hit? No thanks

>
> > And of course, a Sufficiently Smart Compiler could detect
> > (theoretically, of course :)) that this is all just dummy ops and
> > remove them.
>
> No, it can't.  Because the JLS says so.  IMVHO it'd be much better to
> stop trying to guess what a compiler might do and simply write in the
> language instead.

JLS prescribes observable behavior not exact steps.

>
> > In my opinion, the current lack of optimization (accidental or not)
> > should be somehow encoded/made intentional.
>
> I have in the past argued that methods of classes with finalizers
> should automagically extend the lifetime of the "this" object.
> However, I was on the losing side, and reachabilityFence() is the
> compromise result.  That's okay, really: it solves the practical
> problem.
>
> > Perhaps treat Unsafe::anything() as a full compiler optimization
> > fence, if it's not already.
>
> That one really is a no-hoper.  The idea of NIO ByteBuffers is "as
> fast as C" and full fences would be a pretty major regression.

Maybe you missed the "compiler" part.  I'm suggesting a compiler-only fence.  And I like how you suggested updating a global volatile above but here a full fence (which isn't what I proposed) is a no-hoper.

>
> Andrew.


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Alexandre De Champeaux

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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




--
Alexandre

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by David Holmes-6

sent from my phone
On Dec 9, 2015 6:11 AM, "David Holmes" <[hidden email]> wrote:
>
> Andrew Haley writes:
> > On 09/12/15 09:21, David Holmes wrote:
> >
> > > The happens-before requirement was deliberately set the way it is to
> > > show that an object can not be finalized before its construction has
> > > completed (whether normally or abnormally). So I would agree with
> > > Justin that it should not be possible for an object to be finalized
> > > before construction has completed - regardless of potential compiler
> > > optimizations etc.
> >
> > Hmmm, okay.  It might be that I misremembered or perhaps it was a bug,
> > but I don't think so.  Is there really a happens-before relationship
> > between a constructor of an object with no volatile or final fields
> > and its finalizer?
>
> My recollection - and I've been trying to find this in the JavaMemoryModel
> archives - is that the happens-before between constructor and finalizer was
> introduced because the normal "trick" of using synchronization was very
> counter-intuitive in constructors given the object has not been published.
>
> That said, A happens-before B simply establishes visibility/ordering rules
> for the case when A occurs before B. It says nothing about B occurring
> before A, or B and A overlapping. Other mechanisms have to force the
> temporal order. So it may be I misspoke my support of Justin's position. :(

I have a different interpretation, akin to your previous answer.  There's very little point in stating that ctor happens-before finalization if that's not even the case always (i.e. they overlap or occur in reverse order) - how would that be meaningful?

>
> David
> -----
>
>
> > Andrew.
> > _______________________________________________
> > Concurrency-interest mailing list
> > [hidden email]
> > http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Vitaly Davidovich
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? 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.

On Wed, Dec 9, 2015 at 10:57 AM, Gil Tene <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest




--
Alexandre


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich

sent from my phone
On Dec 9, 2015 11:34 AM, "Gil Tene" <[hidden email]> wrote:
>
>
>
> Sent from Gil's iPhone
>
> On Dec 9, 2015, at 8:16 AM, Vitaly Davidovich <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>>>>>> [hidden email]
>>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Concurrency-interest mailing list
>>>>>>>> [hidden email]
>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Concurrency-interest mailing list
>>>>>> [hidden email]
>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alexandre
>>
>>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by Vitaly Davidovich
On 12/09/2015 04:16 PM, Vitaly Davidovich 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?

It would, but it would hurt performance because of increased register
pressure.

Andrew.

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Gil Tene-2

> On Dec 9, 2015, at 9:18 AM, Andrew Haley <[hidden email]> wrote:
>
> On 12/09/2015 04:16 PM, Vitaly Davidovich 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?
>
> It would, but it would hurt performance because of increased register
> pressure.
Actually, it would only add a stack slot. And a spill into it if needed.

> Andrew.
>


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Vitaly Davidovich
I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don't do it. 

This is why I suggested to ask the Hotspot compiler devs who may be able to answer this (relatively) quickly and definitively, or at least as definitive as we'll get at this point.  Jason's example? You mean Alexandre's? The only case demonstrated in this thread that actually segfaults is what Alexandre showed, but as mentioned a few times, it's slightly different from DBB.

I'm not saying I don't believe we're hanging on by a thread here, but I'd just like to hear confirmation of this from someone that works with that code on a daily basis.  Perhaps I've had a bit too much speculation at this point :).

And how would that hurt?
If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

Performance conversations with interpreter in them are meaningless - I don't care about interpreter here :). 

And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

You're retaining memory that could otherwise be cleaned up much sooner.  It's an optimization that would be missed.  Whether it makes any difference, I don't know, it would depend on circumstances.

The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

This assumes reachabilityFence is eventually optimized into a `marker` for compiler only.  Right now, as we've discussed, it's an actual call with prohibited inlining.

BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...
 
Yes, agreed here.  But really, the current inlining heuristics are terrible -- that's a big problem on its own.  reachabilityFence() may aggravate it, which is why I raised that point earlier.
 

On Wed, Dec 9, 2015 at 12:14 PM, Gil Tene <[hidden email]> wrote:


Sent from Gil's iPhone

On Dec 9, 2015, at 8:45 AM, Vitaly Davidovich <[hidden email]> wrote:

sent from my phone
On Dec 9, 2015 11:34 AM, "Gil Tene" <[hidden email]> wrote:
>
>
>
> Sent from Gil's iPhone
>
> On Dec 9, 2015, at 8:16 AM, Vitaly Davidovich <[hidden email]> 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?

I don't deal in facts, I just deal in suppositions ;-)

I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don'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?


And how would that hurt?

If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...

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

The bug technically exists in a non-inlined DBB.get(). Although you are right that it probably won't appear without inlining (the safepoint would be eliminated in the non-inlined method).

But since keeping 'this' alive to the end of those non-inlined methods has no performance downside, there is no need to identify places where this shouldn't be applied...

And within a longer compiled method that includes inlined methods, the 'this' of each inlined method doesn't need to stay reachable to the end of the long containing method. It only needs to stay alive until after the last instruction that originates from that specific inlined method (which is basically what a reachabilityFence in an explicit try...finally within that method would ensure).

> To be specific: the same semantic premature loss of reachability issue exist 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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>>>>>> [hidden email]
>>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Concurrency-interest mailing list
>>>>>>>> [hidden email]
>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Concurrency-interest mailing list
>>>>>> [hidden email]
>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alexandre
>>
>>



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Andrew Haley
As Gil said, it should be spilled as there are no actual uses of it.

On Wed, Dec 9, 2015 at 12:18 PM, Andrew Haley <[hidden email]> wrote:
On 12/09/2015 04:16 PM, Vitaly Davidovich 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?

It would, but it would hurt performance because of increased register
pressure.

Andrew.



_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Alexandre De Champeaux
In reply to this post by Vitaly Davidovich
 
This is why I suggested to ask the Hotspot compiler devs who may be able to answer this (relatively) quickly and definitively, or at least as definitive as we'll get at this point.  Jason's example? You mean Alexandre's? The only case demonstrated in this thread that actually segfaults is what Alexandre showed, but as mentioned a few times, it's slightly different from DBB.

 
Well it's not that different from the get() method of a DBB. 
If you like, you can replace the copy method of my previous example with something like

public byte[] copy() {
final byte[] a = new byte[1];
final long addr = this.addr;
if (ctr++ >= 31_000l * 1000) {
// Counter is there to wait for JIT compilation
System.out.println("Must be compiled now, start doing some GCs " + ctr);
// Here we simulate a safepoint, and a GC at this safepoint
System.gc();
try {
// And here we wait for the reference handler to perform the cleaning to simulate a fast cleaning.
Thread.sleep(1000);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
a[0] = unsafe.getByte(addr);
return a;
}

This crashes as well.
 
Alexandre

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
This is no different than your previous examples in spirit.  Your explicit triggering of GC is contorting the real thing we want to check.  You're also using a loop with a long counter, which will automatically place safepoint polls in that loop.  I'm trying to understand how DBB.get() happens to work today, not how the above doesn't as it's obvious to me why it doesn't. :)

On Wed, Dec 9, 2015 at 12:39 PM, Alexandre De Champeaux <[hidden email]> wrote:
 
This is why I suggested to ask the Hotspot compiler devs who may be able to answer this (relatively) quickly and definitively, or at least as definitive as we'll get at this point.  Jason's example? You mean Alexandre's? The only case demonstrated in this thread that actually segfaults is what Alexandre showed, but as mentioned a few times, it's slightly different from DBB.

 
Well it's not that different from the get() method of a DBB. 
If you like, you can replace the copy method of my previous example with something like

public byte[] copy() {
final byte[] a = new byte[1];
final long addr = this.addr;
if (ctr++ >= 31_000l * 1000) {
// Counter is there to wait for JIT compilation
System.out.println("Must be compiled now, start doing some GCs " + ctr);
// Here we simulate a safepoint, and a GC at this safepoint
System.gc();
try {
// And here we wait for the reference handler to perform the cleaning to simulate a fast cleaning.
Thread.sleep(1000);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
}
}
a[0] = unsafe.getByte(addr);
return a;
}

This crashes as well.
 
Alexandre


_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Gil Tene-2
In reply to this post by Vitaly Davidovich

On Dec 9, 2015, at 9:26 AM, Vitaly Davidovich <[hidden email]> wrote:

I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don't do it. 

This is why I suggested to ask the Hotspot compiler devs who may be able to answer this (relatively) quickly and definitively, or at least as definitive as we'll get at this point.  Jason's example? You mean Alexandre's? The only case demonstrated in this thread that actually segfaults is what Alexandre showed, but as mentioned a few times, it's slightly different from DBB.

Yes. s/Jason/Alexandre/g in my above reference to the example. Sorry for the mixup.

But that example is not different from DBB.get(). Both have a safepoint between the last use if 'this' and a call on unsafe on an address derived from 'this' in the past. Alexandre's simply forces reproduction by preventing the compiler from getting rid of the safepoint. And an inlined DBB.get() can have the same happen to it in various ways.

Alexandre's example is an existence proof that the current compilers to do extend the reachability of 'this' to the end of a method when an unsafe call is included in the method. And also that they do not extend the reachability of 'this' to beyond the unsafe call. If they did either one of those, the example would not trigger a SEGV.


I'm not saying I don't believe we're hanging on by a thread here, but I'd just like to hear confirmation of this from someone that works with that code on a daily basis.  Perhaps I've had a bit too much speculation at this point :).

And how would that hurt?
If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

Performance conversations with interpreter in them are meaningless - I don't care about interpreter here :). 

An expectation that parts of the heap would be collected "better" when compiled than when interpreted is a semantic discussion. I assume that you would not complain about an OOM exception being thrown based on this.


And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

You're retaining memory that could otherwise be cleaned up much sooner.  It's an optimization that would be missed.  Whether it makes any difference, I don't know, it would depend on circumstances.

If you are referring to the extremely-indirect benefit of more objects being released by a GC cycle if it happens to trigger in the middle of this method, you are probably technically right, in the sense that it is a non-zero. But since those effects are dwarfed (by 4+ orders of magnitude) compared to e.g. a choice of starting a CMS mark pass a few milliseconds earlier or later, I doubt that we can even discuss them as a "performance" effect.


The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

This assumes reachabilityFence is eventually optimized into a `marker` for compiler only.  Right now, as we've discussed, it's an actual call with prohibited inlining.

Since I expect reachabilityFence to be used to actually fix all those bugs in DBB, I also expect that it will not remain un-intrinsified in implementation. 

I would not suggest implicit behavior unless it were implemented purely as a keep-this-oop-alive-in-this-frame-until-point-X intrinsic that has no other cost beyond the stack slot remaining occupied and a potential spill into that slot. But once that is implemented (and I expect that it will be), I think that the a change to implicitly extend the reachability 'this' to the end of all instance method (with no exception) will be the right way to go, even if it were not spec'ed that way in the JLS and JVM spec. It is certainly allowed behavior, and implementing it (even without the spec changes) would allow us to avoid fixing all those bugs one-by-one in the JDK, and tripping those bugs in user code.


BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...
 
Yes, agreed here.  But really, the current inlining heuristics are terrible -- that's a big problem on its own.  reachabilityFence() may aggravate it, which is why I raised that point earlier.
 

On Wed, Dec 9, 2015 at 12:14 PM, Gil Tene <[hidden email]> wrote:


Sent from Gil's iPhone

On Dec 9, 2015, at 8:45 AM, Vitaly Davidovich <[hidden email]> wrote:


sent from my phone
On Dec 9, 2015 11:34 AM, "Gil Tene" <[hidden email]> wrote:
>
>
>
> Sent from Gil's iPhone
>
> On Dec 9, 2015, at 8:16 AM, Vitaly Davidovich <[hidden email]> 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?

I don't deal in facts, I just deal in suppositions ;-)

I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don'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?


And how would that hurt?

If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...

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

The bug technically exists in a non-inlined DBB.get(). Although you are right that it probably won't appear without inlining (the safepoint would be eliminated in the non-inlined method).

But since keeping 'this' alive to the end of those non-inlined methods has no performance downside, there is no need to identify places where this shouldn't be applied...

And within a longer compiled method that includes inlined methods, the 'this' of each inlined method doesn't need to stay reachable to the end of the long containing method. It only needs to stay alive until after the last instruction that originates from that specific inlined method (which is basically what a reachabilityFence in an explicit try...finally within that method would ensure).

> To be specific: the same semantic premature loss of reachability issue exist 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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>>>>>> [hidden email]
>>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Concurrency-interest mailing list
>>>>>>>> [hidden email]
>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Concurrency-interest mailing list
>>>>>> [hidden email]
>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alexandre
>>
>>




_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Gil Tene-2
In reply to this post by Doug Lea
[re-sending since it appears that my previous e-mails were not making it into the list earlier (my fault)].

> On Dec 9, 2015, at 5:11 AM, Doug Lea <[hidden email]> wrote:
>
> On 12/09/2015 05:33 AM, Andrew Haley wrote:
>
>>> In my opinion, the current lack of optimization (accidental or not)
>>> should be somehow encoded/made intentional.
>>
>> I have in the past argued that methods of classes with finalizers
>> should automagically extend the lifetime of the "this" object.
>> However, I was on the losing side, and reachabilityFence() is the
>> compromise result.  That's okay, really: it solves the practical
>> problem.
>
> I don't think anyone "lost" these arguments, but further action
> was postponed at least in the context of JMM-related updates.
>
> Among the options is to introduce a @Finalized annotation
> for a field of an object. Front-end compilers would
> then insert reachabilityFence() after each use. Not necessarily
> immediately after though. Optimal placement is non-trivial, which
> is one reason this wasn't widely agreed on. It would be great
> if some IDE's tried out this kind of support inside IDEs though.
Since this issue is in no way contained to finalizers, and effects all phantom/weak/soft refs and cleaners, anything that only applies to classes with finalizers would not be sufficient. E.g. DBB would still need to be fixed.

I also don't think that an annotation on a field that implies a reachability fence (regardless office we name it) would suffice or help much. DBB is a great example of why I think this: the buffer management in DBB is an internal concern, and the current reachability bugs in its get() and put() implementations are internal implementation bugs. As such, it would clearly make no sense for a user of DBB to place an @Finalized (or equivalent) annotation on their instance fields that refers to a DBB. But inside of DBB, field annotation can't be used since there is no field to annotate, since it is 'this' that needs its reachability extended to the end of its instance methods.

My take on this would be to change the JLS and JVM spec to state that 'this' remains reachable until after the last instruction of every instance method. Period. This would basically equate to placing an implicit reachability fence at all exist points of every instance method (I.e. The equivalent of a try...finally). The "nice״ thing about such an implicit reachability fence is that it does not really defeat any optimizations, as it only serves to extend the lifetime of an oop (so potentially pressing slightly harder on the register allocator). IMO this would auto-magically fix lots of current rarely occurring bugs (like those on DBB, but also in user code), and prevent many future ones.

For now I'm happy to see reachabilityFence, because I'm convinced that we have many resource cleanup race bugs in the JDK that are currently in fixable without it. But I think we should consider this implementing this implicit behavior (extending 'this' reachability to the end of all instance methods) even if the spec is not changed, because it would save us the manual work of fixing all the existing bugs by adding reachability fences. And would obviously provide better coverage than anything we could do manually.

>
> See discussions on jmm-dev list mostly in August 2014.
> http://mail.openjdk.java.net/pipermail/jmm-dev/
>
> (Also, someday, the JLS probably should be updated to explicitly
> mention impact of reachabilityFence. The wording change
> would basically be the same as the method spec, but a few other
> clarifications done at the same time might be helpful.
>
> -Doug
>
>
> _______________________________________________
> Concurrency-interest mailing list
> [hidden email]
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest

_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

signature.asc (859 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Gil Tene-2
Alexandre's example is an existence proof that the current compilers to do extend the reachability of 'this' to the end of a method when an unsafe call is included in the method. And also that they do not extend the reachability of 'this' to beyond the unsafe call. If they did either one of those, the example would not trigger a SEGV.

Ok, I buy that.  I can't help but wonder how DBB authors decided it's ok to release it to the public like this though.  Granted, I can't think of a (realistic) scenario where allocating a DBB that's immediately garbage is normal.

An expectation that parts of the heap would be collected "better" when compiled than when interpreted is a semantic discussion. I assume that you would not complain about an OOM exception being thrown based on this.

It's semantic to an extent; java would be unusable if there was no JIT.  I wouldn't complain about an OOM, but I may complain if the performance profile changes from today.  Once a daemon's hot paths are compiled, throughput goes up which means more GC pressure per unit of time than interpreter.  If all of a sudden the JIT was less aggressive across the board, I suspect it may rub some scenarios the wrong way.
 
If you are referring to the extremely-indirect benefit of more objects being released by a GC cycle if it happens to trigger in the middle of this method, you are probably technically right, in the sense that it is a non-zero. But since those effects are dwarfed (by 4+ orders of magnitude) compared to e.g. a choice of starting a CMS mark pass a few milliseconds earlier or later, I doubt that we can even discuss them as a "performance" effect.

I'd rather not conflate these things.  Otherwise, I can argue that micro optimizing, e.g., DBB.get() is irrelevant because there are plenty of other perf cliffs in the system.

Since I expect reachabilityFence to be used to actually fix all those bugs in DBB, I also expect that it will not remain un-intrinsified in implementation.

Let's hope so.

I would not suggest implicit behavior unless it were implemented purely as a keep-this-oop-alive-in-this-frame-until-point-X intrinsic that has no other cost beyond the stack slot remaining occupied and a potential spill into that slot. But once that is implemented (and I expect that it will be), I think that the a change to implicitly extend the reachability 'this' to the end of all instance method (with no exception) will be the right way to go, even if it were not spec'ed that way in the JLS and JVM spec. It is certainly allowed behavior, and implementing it (even without the spec changes) would allow us to avoid fixing all those bugs one-by-one in the JDK, and tripping those bugs in user code.

If the choice is between this vs crashes (or worse, reads of free'd but still mapped memory that now has garbage/wrong data), then I agree.  I guess another benefit would be that it would reduce need/use of reachabilityFence, and thus having to reason about these things in fewer places.

On Wed, Dec 9, 2015 at 12:49 PM, Gil Tene <[hidden email]> wrote:

On Dec 9, 2015, at 9:26 AM, Vitaly Davidovich <[hidden email]> wrote:

I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don't do it. 

This is why I suggested to ask the Hotspot compiler devs who may be able to answer this (relatively) quickly and definitively, or at least as definitive as we'll get at this point.  Jason's example? You mean Alexandre's? The only case demonstrated in this thread that actually segfaults is what Alexandre showed, but as mentioned a few times, it's slightly different from DBB.

Yes. s/Jason/Alexandre/g in my above reference to the example. Sorry for the mixup.

But that example is not different from DBB.get(). Both have a safepoint between the last use if 'this' and a call on unsafe on an address derived from 'this' in the past. Alexandre's simply forces reproduction by preventing the compiler from getting rid of the safepoint. And an inlined DBB.get() can have the same happen to it in various ways.

Alexandre's example is an existence proof that the current compilers to do extend the reachability of 'this' to the end of a method when an unsafe call is included in the method. And also that they do not extend the reachability of 'this' to beyond the unsafe call. If they did either one of those, the example would not trigger a SEGV.


I'm not saying I don't believe we're hanging on by a thread here, but I'd just like to hear confirmation of this from someone that works with that code on a daily basis.  Perhaps I've had a bit too much speculation at this point :).

And how would that hurt?
If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

Performance conversations with interpreter in them are meaningless - I don't care about interpreter here :). 

An expectation that parts of the heap would be collected "better" when compiled than when interpreted is a semantic discussion. I assume that you would not complain about an OOM exception being thrown based on this.


And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

You're retaining memory that could otherwise be cleaned up much sooner.  It's an optimization that would be missed.  Whether it makes any difference, I don't know, it would depend on circumstances.

If you are referring to the extremely-indirect benefit of more objects being released by a GC cycle if it happens to trigger in the middle of this method, you are probably technically right, in the sense that it is a non-zero. But since those effects are dwarfed (by 4+ orders of magnitude) compared to e.g. a choice of starting a CMS mark pass a few milliseconds earlier or later, I doubt that we can even discuss them as a "performance" effect.


The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

This assumes reachabilityFence is eventually optimized into a `marker` for compiler only.  Right now, as we've discussed, it's an actual call with prohibited inlining.

Since I expect reachabilityFence to be used to actually fix all those bugs in DBB, I also expect that it will not remain un-intrinsified in implementation. 

I would not suggest implicit behavior unless it were implemented purely as a keep-this-oop-alive-in-this-frame-until-point-X intrinsic that has no other cost beyond the stack slot remaining occupied and a potential spill into that slot. But once that is implemented (and I expect that it will be), I think that the a change to implicitly extend the reachability 'this' to the end of all instance method (with no exception) will be the right way to go, even if it were not spec'ed that way in the JLS and JVM spec. It is certainly allowed behavior, and implementing it (even without the spec changes) would allow us to avoid fixing all those bugs one-by-one in the JDK, and tripping those bugs in user code.


BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...
 
Yes, agreed here.  But really, the current inlining heuristics are terrible -- that's a big problem on its own.  reachabilityFence() may aggravate it, which is why I raised that point earlier.
 

On Wed, Dec 9, 2015 at 12:14 PM, Gil Tene <[hidden email]> wrote:


Sent from Gil's iPhone

On Dec 9, 2015, at 8:45 AM, Vitaly Davidovich <[hidden email]> wrote:


sent from my phone
On Dec 9, 2015 11:34 AM, "Gil Tene" <[hidden email]> wrote:
>
>
>
> Sent from Gil's iPhone
>
> On Dec 9, 2015, at 8:16 AM, Vitaly Davidovich <[hidden email]> 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?

I don't deal in facts, I just deal in suppositions ;-)

I'm not saying it's a fact. I'm just saying that ice never seen any indication of the compilers trying to do this (and it's certainly not required by any spec). And Jason's example seems to demonstrate at least one case where they actually don'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?


And how would that hurt?

If you were running interpreted, you would have that behavior anyway, so there is no valid semantic argument against it. E.g. the "but the GC won't collect 'this' for minutes in a long running loop when it could have" argument wouldn't fly here.

And as for performance, keeping 'this' live to the end of the frame when it would otherwise have been untraceable would at worst eat up an additional stack slot in your frame that could have otherwise been recycled (it wouldn't actually eat up a register, because the register allocator would/should just spill it to the stack of the register was needed).

The main downside has to do with teaching the compilers to actually do this. It's against their nature to keep otherwise dead oops in registers or sack locations, so they need to be told to do this somehow. But once reachabilityFence is implemented, the compilers have been taught how, and now it's just a question of where.

BTW, the implicit approach (for 'this' in all instance methods) is probably less performance-intrusive than manually adding referenceFence calls in e.g. DBB, because the implicit approach would not add any bytecodes to the affected methods, and would thereby avoid messing with currently working (but sometimes fragile) in lining heuristics... And since DBB get and put calls are often very performance sensitive, this could be a real issue...

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

The bug technically exists in a non-inlined DBB.get(). Although you are right that it probably won't appear without inlining (the safepoint would be eliminated in the non-inlined method).

But since keeping 'this' alive to the end of those non-inlined methods has no performance downside, there is no need to identify places where this shouldn't be applied...

And within a longer compiled method that includes inlined methods, the 'this' of each inlined method doesn't need to stay reachable to the end of the long containing method. It only needs to stay alive until after the last instruction that originates from that specific inlined method (which is basically what a reachabilityFence in an explicit try...finally within that method would ensure).

> To be specific: the same semantic premature loss of reachability issue exist 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 <[hidden email]> 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 <[hidden email]> 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" <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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
>>>>>>>>> [hidden email]
>>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Concurrency-interest mailing list
>>>>>>>> [hidden email]
>>>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Concurrency-interest mailing list
>>>>>> [hidden email]
>>>>>> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Alexandre
>>
>>





_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by Gil Tene-2
On 12/09/2015 05:23 PM, Gil Tene wrote:

>
>> On Dec 9, 2015, at 9:18 AM, Andrew Haley <[hidden email]> wrote:
>>
>> On 12/09/2015 04:16 PM, Vitaly Davidovich 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?
>>
>> It would, but it would hurt performance because of increased register
>> pressure.
>
> Actually, it would only add a stack slot. And a spill into it if needed.

Hesitant as I am to argue with you, I'm not so sure.  There'd
definitely have to be a USE in order to keep the oop alive, and C2
would surely try not to spill the oop.  The USE would be at the end of
every method.  Unless there's some sort of "sink" node in the ideal
graph which keeps an oop alive but does not refill from a stack slot,
and I don't think there is.

Andrew.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by Vitaly Davidovich
On 12/09/2015 02:36 PM, Vitaly Davidovich wrote:
> sent from my phone
> On Dec 9, 2015 5:33 AM, "Andrew Haley" <[hidden email]> wrote:
>>
>> On 08/12/15 19:17, Vitaly Davidovich wrote:
>>>> [me:]

>>> So now I'm going to write to dummy fields? And what are you going to
>>> do with it in a finalizer?
>>
>> Update a global volatile.
>
> And take perf hit? No thanks

I don't think it'd be measurable.  A global volatile write is pretty
cheap.  And I'm sure there are several such writes when queuing a
finalizer.

>>> And of course, a Sufficiently Smart Compiler could detect
>>> (theoretically, of course :)) that this is all just dummy ops and
>>> remove them.
>>
>> No, it can't.  Because the JLS says so.  IMVHO it'd be much better to
>> stop trying to guess what a compiler might do and simply write in the
>> language instead.
>
> JLS prescribes observable behavior not exact steps.

Well, yes.  Indeed it does.

>>> In my opinion, the current lack of optimization (accidental or not)
>>> should be somehow encoded/made intentional.
>>
>> I have in the past argued that methods of classes with finalizers
>> should automagically extend the lifetime of the "this" object.
>> However, I was on the losing side, and reachabilityFence() is the
>> compromise result.  That's okay, really: it solves the practical
>> problem.
>>
>>> Perhaps treat Unsafe::anything() as a full compiler optimization
>>> fence, if it's not already.
>>
>> That one really is a no-hoper.  The idea of NIO ByteBuffers is "as
>> fast as C" and full fences would be a pretty major regression.
>
> Maybe you missed the "compiler" part.

Not at all.

> I'm suggesting a compiler-only fence.  And I like how you suggested
> updating a global volatile above but here a full fence (which isn't
> what I proposed) is a no-hoper.

The global volatile gets hit once, when the object is finalized.  Not
at every access, which is what you'd have to do with a compiler
barrier.  Compiler barriers have a huge effect because they break many
loop optimizations.  That's a no-hoper.

Andrew.
_______________________________________________
Concurrency-interest mailing list
[hidden email]
http://cs.oswego.edu/mailman/listinfo/concurrency-interest
123456 ... 8