DirectByteBuffers and reachabilityFence

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

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
On 12/08/2015 02:40 PM, 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).

Okay.  Can you tell me the relevance of this, though?

> It's not just HotSpot, though: some VMs are even more aggressive, and
>
> Which java VMs are these? Just curious.

IBM's J9.

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

Or simply determine that nothing depends on position, so an update to
it doesn't have to be generated.

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

Not really: the JMM determines what is possible.  It's not just
"anything".

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

It might already have happened.  The problem with reasoning in such a
fragile way is that things get broken by accident.  The current Unsafe
support works by accident: as we have seen, copyMemory works because
position is updated after the call to Unsafe.  People who are
reasoning in terms of the compiler don't necessarily think of all the
incorrect things library authors might have done.

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 M. Lloyd-3
In reply to this post by Vitaly Davidovich
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
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Vitaly Davidovich
In reply to this post by Andrew Haley
Okay.  Can you tell me the relevance of this, though?

Relevance is if code works correctly in interpreter then, practically speaking, compiler tries to not break it.  Of course this isn't always the case (e.g. compiler hoisting reads of a non-volatile field that should be volatile), but is generally true. 

Or simply determine that nothing depends on position, so an update to
it doesn't have to be generated.

This is irrelevant.  There are, today, artifacts the optimizer leaves behind even if it eliminates certain other operations to preserve behavior, warranted by spec or simply being cautious about breaking code.

Not really: the JMM determines what is possible.  It's not just
"anything".

Yes, "anything" was hyperbole.

It might already have happened.  The problem with reasoning in such a
fragile way is that things get broken by accident.  The current Unsafe
support works by accident: as we have seen, copyMemory works because
position is updated after the call to Unsafe.  People who are
reasoning in terms of the compiler don't necessarily think of all the
incorrect things library authors might have done.

There are other operations in DBB that touch native memory that appear to work without any position update (i.e. the various getters).  Again, I'm not advocating reasoning like this for new code that someone may write.  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.  By definition, this is speculation unless you're expecting me to provide a full trace through the compiler optimization pipeline.  At no point did I advise/suggest that someone should reason like this for their code, I was merely trying to use educated guesses as to why it works keeping compiler optimizations in mind.

As to the theoretical vs practical aspect, I agree that there's nothing holding this together spec/theory wise; afterall, I'm quite happy that reachabilityFence is being added (don't particularly like that name, but whatever).  But if you create a spec conforming JVM implementation today that segfaults in DBB, congrats but nobody is going to use it.  Moreover, once reachabilityFence is added and sprinkled in all the appropriate JDK places, there may be a time when someone advertently or inadvertently makes a compiler optimization that will break DBB-like clones in user code.  My hunch, given the mindset of java and emphasis on not breaking code, even code that's misbehaved and not conforming to spec, is that such optimization will not go forward.  There are already cases where JVM treads carefully to cater to java code out in the wild; the fact that final fields are not treated as constants due to fear of reflection updates is a prime example which actually could have tangible performance benefits if it weren't so.  AFAIK, there's nothing in the spec that states it's legal to update final fields.

We can agree on the spec all we want, but the reality/practical aspects are more nuanced. 




On Tue, Dec 8, 2015 at 10:17 AM, Andrew Haley <[hidden email]> wrote:
On 12/08/2015 02:40 PM, 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).

Okay.  Can you tell me the relevance of this, though?

> It's not just HotSpot, though: some VMs are even more aggressive, and
>
> Which java VMs are these? Just curious.

IBM's J9.

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

Or simply determine that nothing depends on position, so an update to
it doesn't have to be generated.

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

Not really: the JMM determines what is possible.  It's not just
"anything".

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

It might already have happened.  The problem with reasoning in such a
fragile way is that things get broken by accident.  The current Unsafe
support works by accident: as we have seen, copyMemory works because
position is updated after the call to Unsafe.  People who are
reasoning in terms of the compiler don't necessarily think of all the
incorrect things library authors might have done.

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 David M. Lloyd-3
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
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by Vitaly Davidovich
On 12/08/2015 03:35 PM, Vitaly Davidovich wrote:
>>
>> Okay.  Can you tell me the relevance of this, though?
>
> Relevance is if code works correctly in interpreter then, practically
> speaking, compiler tries to not break it.  Of course this isn't always the
> case (e.g. compiler hoisting reads of a non-volatile field that should be
> volatile), but is generally true.

I don't think it's generally true.

>> It might already have happened.  The problem with reasoning in such
>> a fragile way is that things get broken by accident.  The current
>> Unsafe support works by accident: as we have seen, copyMemory works
>> because position is updated after the call to Unsafe.  People who
>> are reasoning in terms of the compiler don't necessarily think of
>> all the incorrect things library authors might have done.
>
> There are other operations in DBB that touch native memory that
> appear to work without any position update (i.e. the various
> getters).  Again, I'm not advocating reasoning like this for new
> code that someone may write.  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.

And in that case the answer is, I suspect, that the optimizer doesn't
know that the position field is unused by the finalizer.  If it did
know this, then you'd have early finalization.  As for the other
cases, it's not worth speculating without looking at code.

> By definition, this is speculation unless you're expecting me to
> provide a full trace through the compiler optimization pipeline.  At
> no point did I advise/suggest that someone should reason like this
> for their code, I was merely trying to use educated guesses as to
> why it works keeping compiler optimizations in mind.

Me too.  But surely why it works isn't really all that interesting:
what is much more interesting, and relevant to this list, is that it
may stop working, may already not work, or may not work in some VMs.

> As to the theoretical vs practical aspect, I agree that there's
> nothing holding this together spec/theory wise; afterall, I'm quite
> happy that reachabilityFence is being added (don't particularly like
> that name, but whatever).  But if you create a spec conforming JVM
> implementation today that segfaults in DBB, congrats but nobody is
> going to use it.

I don't see why: it'd require a patch to DBB, but people don't ship
JVMs without a copy of rt.jar (or somesuch) anyway.

> Moreover, once reachabilityFence is added and sprinkled in all the
> appropriate JDK places, there may be a time when someone advertently
> or inadvertently makes a compiler optimization that will break
> DBB-like clones in user code.  My hunch, given the mindset of java
> and emphasis on not breaking code, even code that's misbehaved and
> not conforming to spec, is that such optimization will not go
> forward.

We have very different hunches!

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
I don't think it's generally true.

So your claim is that, generally speaking, if something works a certain (observable) way in interpreter it's not guaranteed to work in same (observable) way with compiled code, except quicker? I find that hard to believe based on various compiler-dev threads I've read over the years, but no point in debating this.

And in that case the answer is, I suspect, that the optimizer doesn't
know that the position field is unused by the finalizer.  If it did
know this, then you'd have early finalization.  As for the other
cases, it's not worth speculating without looking at code.

The question is why do the various getters work in the face of early finalization/reclaim given they don't touch internal state? I don't have a good answer for this other than compiler is careful about optimizing around Unsafe operations (it does know about unsafe memory accesses, that I know for sure).

Me too.  But surely why it works isn't really all that interesting:
what is much more interesting, and relevant to this list, is that it
may stop working, may already not work, or may not work in some VMs.

Whether why it works today is interesting or not is subjective.  Whether it's relevant to this list -- is this a theory only list? I don't think so.  Theoretical discussions are good and form a basis for implementation, but implementation details are also interesting (and have material performance implications), at least to me.

I don't see why: it'd require a patch to DBB, but people don't ship
JVMs without a copy of rt.jar (or somesuch) anyway.

See my comment about final field treatment as it stands today.  You can patch JDK, but you'll likely break some user code that was "following" DBB as a model.  Personally, I prefer aggressive optimizations and willing to fix code if I did something wrong, but in this case there was no good way to fix the code prior to writing it.  This is not a spec-wise decision/situation, this is purely practical implications of breaking user code.  This whole thing begs the rhetorical question then -- why was this functionality released given it's rife with lifetime issues? That ship has sailed now, and I'd hate to be the one explaining to users that some optimizer change now segfaults their VM.

We have very different hunches!

Looks like it; let's circle back to this thread if/when an optimization/code change breaks user code, which works today, of this nature -- I'll be ready with the popcorn. :)


On Tue, Dec 8, 2015 at 11:21 AM, Andrew Haley <[hidden email]> wrote:
On 12/08/2015 03:35 PM, Vitaly Davidovich wrote:
>>
>> Okay.  Can you tell me the relevance of this, though?
>
> Relevance is if code works correctly in interpreter then, practically
> speaking, compiler tries to not break it.  Of course this isn't always the
> case (e.g. compiler hoisting reads of a non-volatile field that should be
> volatile), but is generally true.

I don't think it's generally true.

>> It might already have happened.  The problem with reasoning in such
>> a fragile way is that things get broken by accident.  The current
>> Unsafe support works by accident: as we have seen, copyMemory works
>> because position is updated after the call to Unsafe.  People who
>> are reasoning in terms of the compiler don't necessarily think of
>> all the incorrect things library authors might have done.
>
> There are other operations in DBB that touch native memory that
> appear to work without any position update (i.e. the various
> getters).  Again, I'm not advocating reasoning like this for new
> code that someone may write.  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.

And in that case the answer is, I suspect, that the optimizer doesn't
know that the position field is unused by the finalizer.  If it did
know this, then you'd have early finalization.  As for the other
cases, it's not worth speculating without looking at code.

> By definition, this is speculation unless you're expecting me to
> provide a full trace through the compiler optimization pipeline.  At
> no point did I advise/suggest that someone should reason like this
> for their code, I was merely trying to use educated guesses as to
> why it works keeping compiler optimizations in mind.

Me too.  But surely why it works isn't really all that interesting:
what is much more interesting, and relevant to this list, is that it
may stop working, may already not work, or may not work in some VMs.

> As to the theoretical vs practical aspect, I agree that there's
> nothing holding this together spec/theory wise; afterall, I'm quite
> happy that reachabilityFence is being added (don't particularly like
> that name, but whatever).  But if you create a spec conforming JVM
> implementation today that segfaults in DBB, congrats but nobody is
> going to use it.

I don't see why: it'd require a patch to DBB, but people don't ship
JVMs without a copy of rt.jar (or somesuch) anyway.

> Moreover, once reachabilityFence is added and sprinkled in all the
> appropriate JDK places, there may be a time when someone advertently
> or inadvertently makes a compiler optimization that will break
> DBB-like clones in user code.  My hunch, given the mindset of java
> and emphasis on not breaking code, even code that's misbehaved and
> not conforming to spec, is that such optimization will not go
> forward.

We have very different hunches!

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 Vitaly Davidovich
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.

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
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Andrew Haley
In reply to this post by Vitaly Davidovich
On 12/08/2015 04:44 PM, Vitaly Davidovich wrote:

> So your claim is that, generally speaking, if something works a
> certain (observable) way in interpreter it's not guaranteed to work
> in same (observable) way with compiled code, except quicker? I find
> that hard to believe based on various compiler-dev threads I've read
> over the years, but no point in debating this.

Yes, that is my claim.  It's certainly true in the case of concurrent
code, which is the subject of this list.  It's certainly true in the
case of reachability, which is the subject we're discussing.

>> And in that case the answer is, I suspect, that the optimizer doesn't
>> know that the position field is unused by the finalizer.  If it did
>> know this, then you'd have early finalization.  As for the other
>> cases, it's not worth speculating without looking at code.
>
> The question is why do the various getters work in the face of early
> finalization/reclaim given they don't touch internal state? I don't
> have a good answer for this other than compiler is careful about
> optimizing around Unsafe operations (it does know about unsafe
> memory accesses, that I know for sure).

Dunno.  It'd be interesting to try to break a getter and then look at
the code.

>> I don't see why: it'd require a patch to DBB, but people don't ship
>> JVMs without a copy of rt.jar (or somesuch) anyway.
>
> See my comment about final field treatment as it stands today.  You
> can patch JDK, but you'll likely break some user code that was
> "following" DBB as a model.

Indeed.

> Personally, I prefer aggressive optimizations and willing to fix
> code if I did something wrong, but in this case there was no good
> way to fix the code prior to writing it.  This is not a spec-wise
> decision/situation, this is purely practical implications of
> breaking user code.  This whole thing begs the rhetorical question
> then -- why was this functionality released given it's rife with
> lifetime issues?

This warning about object lifetimes and finalizers is part of the
now-twenty-year-old Java Language Specification.  One would hope that
people have had time to get used to it and its implications.

> That ship has sailed now, and I'd hate to be the one explaining to
> users that some optimizer change now segfaults their VM.

It'll only happen with some user abusing Unsafe.  I know perfectly
well what would be said!

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
This warning about object lifetimes and finalizers is part of the
now-twenty-year-old Java Language Specification.  One would hope that
people have had time to get used to it and its implications.

We're going in circles now.  Let me just ask you this -- do you think Oracle would ship a JDK version where code, following the model of DBB, now breaks? The problem is that there was no way to achieve the desired lifetime guarantees with today's and all prior JDK versions.  Why was DBB released then if it's full of subtle gotchas? What are you going to say to users whose code will break, and it will break subtly under particular optimization/code shape scenarios? That they should've followed the 20+ yr old JLS spec, and now that reachabilityFence is out, they should sweep all their code and their 3rd party deps to ensure these subtle lifetimes issues are fixed?

It'll only happen with some user abusing Unsafe.  I know perfectly
well what would be said!

Didn't we just recently experience the hoopla surrounding Unsafe usage? And that was "simply" about hiding Unsafe -- now you're going to crash user code and expect what? Praise? Let's be realistic here and not be dogmatic about what the spec says.  The spec would be fantastic if behavior was consistent, predictable, and enforceable/controllable from user code on Day 1.

Again, there's precedent for existing unspec'd behavior either causing implementations to ensure they don't break the behavior and/or causing the spec to be updated to match behavior.

On Tue, Dec 8, 2015 at 1:15 PM, Andrew Haley <[hidden email]> wrote:
On 12/08/2015 04:44 PM, Vitaly Davidovich wrote:

> So your claim is that, generally speaking, if something works a
> certain (observable) way in interpreter it's not guaranteed to work
> in same (observable) way with compiled code, except quicker? I find
> that hard to believe based on various compiler-dev threads I've read
> over the years, but no point in debating this.

Yes, that is my claim.  It's certainly true in the case of concurrent
code, which is the subject of this list.  It's certainly true in the
case of reachability, which is the subject we're discussing.

>> And in that case the answer is, I suspect, that the optimizer doesn't
>> know that the position field is unused by the finalizer.  If it did
>> know this, then you'd have early finalization.  As for the other
>> cases, it's not worth speculating without looking at code.
>
> The question is why do the various getters work in the face of early
> finalization/reclaim given they don't touch internal state? I don't
> have a good answer for this other than compiler is careful about
> optimizing around Unsafe operations (it does know about unsafe
> memory accesses, that I know for sure).

Dunno.  It'd be interesting to try to break a getter and then look at
the code.

>> I don't see why: it'd require a patch to DBB, but people don't ship
>> JVMs without a copy of rt.jar (or somesuch) anyway.
>
> See my comment about final field treatment as it stands today.  You
> can patch JDK, but you'll likely break some user code that was
> "following" DBB as a model.

Indeed.

> Personally, I prefer aggressive optimizations and willing to fix
> code if I did something wrong, but in this case there was no good
> way to fix the code prior to writing it.  This is not a spec-wise
> decision/situation, this is purely practical implications of
> breaking user code.  This whole thing begs the rhetorical question
> then -- why was this functionality released given it's rife with
> lifetime issues?

This warning about object lifetimes and finalizers is part of the
now-twenty-year-old Java Language Specification.  One would hope that
people have had time to get used to it and its implications.

> That ship has sailed now, and I'd hate to be the one explaining to
> users that some optimizer change now segfaults their VM.

It'll only happen with some user abusing Unsafe.  I know perfectly
well what would be said!

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
Yes, that is my claim.  It's certainly true in the case of concurrent
code, which is the subject of this list.  It's certainly true in the
case of reachability, which is the subject we're discussing.

I missed this part.  For concurrent code, how about the fact that volatile writes in a constructor are effectively treated like final field writes, even though current JMM does not mandate that? Last I heard, a future JMM revision will make it such that existing behavior is actually spec'd simply because lots of existing code relies on it and would break, subtly, if that were to change. 

On Tue, Dec 8, 2015 at 1:28 PM, Vitaly Davidovich <[hidden email]> wrote:
This warning about object lifetimes and finalizers is part of the
now-twenty-year-old Java Language Specification.  One would hope that
people have had time to get used to it and its implications.

We're going in circles now.  Let me just ask you this -- do you think Oracle would ship a JDK version where code, following the model of DBB, now breaks? The problem is that there was no way to achieve the desired lifetime guarantees with today's and all prior JDK versions.  Why was DBB released then if it's full of subtle gotchas? What are you going to say to users whose code will break, and it will break subtly under particular optimization/code shape scenarios? That they should've followed the 20+ yr old JLS spec, and now that reachabilityFence is out, they should sweep all their code and their 3rd party deps to ensure these subtle lifetimes issues are fixed?

It'll only happen with some user abusing Unsafe.  I know perfectly
well what would be said!

Didn't we just recently experience the hoopla surrounding Unsafe usage? And that was "simply" about hiding Unsafe -- now you're going to crash user code and expect what? Praise? Let's be realistic here and not be dogmatic about what the spec says.  The spec would be fantastic if behavior was consistent, predictable, and enforceable/controllable from user code on Day 1.

Again, there's precedent for existing unspec'd behavior either causing implementations to ensure they don't break the behavior and/or causing the spec to be updated to match behavior.

On Tue, Dec 8, 2015 at 1:15 PM, Andrew Haley <[hidden email]> wrote:
On 12/08/2015 04:44 PM, Vitaly Davidovich wrote:

> So your claim is that, generally speaking, if something works a
> certain (observable) way in interpreter it's not guaranteed to work
> in same (observable) way with compiled code, except quicker? I find
> that hard to believe based on various compiler-dev threads I've read
> over the years, but no point in debating this.

Yes, that is my claim.  It's certainly true in the case of concurrent
code, which is the subject of this list.  It's certainly true in the
case of reachability, which is the subject we're discussing.

>> And in that case the answer is, I suspect, that the optimizer doesn't
>> know that the position field is unused by the finalizer.  If it did
>> know this, then you'd have early finalization.  As for the other
>> cases, it's not worth speculating without looking at code.
>
> The question is why do the various getters work in the face of early
> finalization/reclaim given they don't touch internal state? I don't
> have a good answer for this other than compiler is careful about
> optimizing around Unsafe operations (it does know about unsafe
> memory accesses, that I know for sure).

Dunno.  It'd be interesting to try to break a getter and then look at
the code.

>> I don't see why: it'd require a patch to DBB, but people don't ship
>> JVMs without a copy of rt.jar (or somesuch) anyway.
>
> See my comment about final field treatment as it stands today.  You
> can patch JDK, but you'll likely break some user code that was
> "following" DBB as a model.

Indeed.

> Personally, I prefer aggressive optimizations and willing to fix
> code if I did something wrong, but in this case there was no good
> way to fix the code prior to writing it.  This is not a spec-wise
> decision/situation, this is purely practical implications of
> breaking user code.  This whole thing begs the rhetorical question
> then -- why was this functionality released given it's rife with
> lifetime issues?

This warning about object lifetimes and finalizers is part of the
now-twenty-year-old Java Language Specification.  One would hope that
people have had time to get used to it and its implications.

> That ship has sailed now, and I'd hate to be the one explaining to
> users that some optimizer change now segfaults their VM.

It'll only happen with some user abusing Unsafe.  I know perfectly
well what would be said!

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
On 12/08/2015 06:48 PM, Vitaly Davidovich wrote:

>>
>> Yes, that is my claim.  It's certainly true in the case of concurrent
>> code, which is the subject of this list.  It's certainly true in the
>> case of reachability, which is the subject we're discussing.
>
> I missed this part.  For concurrent code, how about the fact that volatile
> writes in a constructor are effectively treated like final field writes,
> even though current JMM does not mandate that? Last I heard, a future JMM
> revision will make it such that existing behavior is actually spec'd simply
> because lots of existing code relies on it and would break, subtly, if that
> were to change.

I'm trying to figure out exactly what point you're making.  In what
sense are volatile writes in a constructor effectively treated like
final field writes?  From the code I've looked at, volatile writes in
a constructor are treated exactly like any other kind of volatile
writes: they're sequentially consistent, and there's nothing stronger
than that.

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

sent from my phone
On Dec 8, 2015 1:54 PM, "Andrew Haley" <[hidden email]> wrote:
>
> On 12/08/2015 06:48 PM, Vitaly Davidovich wrote:
> >>
> >> Yes, that is my claim.  It's certainly true in the case of concurrent
> >> code, which is the subject of this list.  It's certainly true in the
> >> case of reachability, which is the subject we're discussing.
> >
> > I missed this part.  For concurrent code, how about the fact that volatile
> > writes in a constructor are effectively treated like final field writes,
> > even though current JMM does not mandate that? Last I heard, a future JMM
> > revision will make it such that existing behavior is actually spec'd simply
> > because lots of existing code relies on it and would break, subtly, if that
> > were to change.
>
> I'm trying to figure out exactly what point you're making. 

My point, which you're countering, is that JVM sometimes enforces stricter semantics than the spec to satisfy behavior that existing code depends on and doesn't just say "read the spec" to justify breaking changes.

> In what
> sense are volatile writes in a constructor effectively treated like
> final field writes?  From the code I've looked at, volatile writes in
> a constructor are treated exactly like any other kind of volatile
> writes: they're sequentially consistent, and there's nothing stronger
> than that.

Final field prevents subsequent assignment of the newly constructed object from moving above the final field write inside the constructor; volatile write doesn't carry this requirement by JMM spec but gets that treatment in the implementation.

>
> 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/08/2015 06:28 PM, Vitaly Davidovich wrote:
>>
>> This warning about object lifetimes and finalizers is part of the
>> now-twenty-year-old Java Language Specification.  One would hope that
>> people have had time to get used to it and its implications.
>
> We're going in circles now.  Let me just ask you this -- do you
> think Oracle would ship a JDK version where code, following the
> model of DBB, now breaks?

Absolutely, yes.  We already know that very similar code breaks.

> The problem is that there was no way to achieve the desired lifetime
> guarantees with today's and all prior JDK versions.

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.

> Why was DBB released then if it's full of subtle gotchas? What are
> you going to say to users whose code will break, and it will break
> subtly under particular optimization/code shape scenarios? That they
> should've followed the 20+ yr old JLS spec, and now that
> reachabilityFence is out, they should sweep all their code and their
> 3rd party deps to ensure these subtle lifetimes issues are fixed?

Yes, more or less.  It's happened in the past, and it'll continue
to happen.  To begin with people didn't much have to worry about
subtleties to do with finalization and lifetimes, and were bitten
by it, and fixed their code.

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
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?? :) So now I'm going to write to dummy fields? And what are you going to do with it in a finalizer? And of course, a Sufficiently Smart Compiler could detect (theoretically, of course :)) that this is all just dummy ops and remove them.

Yes, more or less.  It's happened in the past, and it'll continue
to happen.  To begin with people didn't much have to worry about
subtleties to do with finalization and lifetimes, and were bitten
by it, and fixed their code.

In that case, please let me know if this ever happens, hopefully on a public mailing list that I can follow along. 

In my opinion, the current lack of optimization (accidental or not) should be somehow encoded/made intentional.  Perhaps treat Unsafe::anything() as a full compiler optimization fence, if it's not already.  At least don't break existing code.  For Alexandre's SimpleBuffer version that broke and breaks today, require them to use reachabilityFence.  This, or something along these lines, is the pragmatic way to address this.
 

On Tue, Dec 8, 2015 at 2:03 PM, Andrew Haley <[hidden email]> wrote:
On 12/08/2015 06:28 PM, Vitaly Davidovich wrote:
>>
>> This warning about object lifetimes and finalizers is part of the
>> now-twenty-year-old Java Language Specification.  One would hope that
>> people have had time to get used to it and its implications.
>
> We're going in circles now.  Let me just ask you this -- do you
> think Oracle would ship a JDK version where code, following the
> model of DBB, now breaks?

Absolutely, yes.  We already know that very similar code breaks.

> The problem is that there was no way to achieve the desired lifetime
> guarantees with today's and all prior JDK versions.

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.

> Why was DBB released then if it's full of subtle gotchas? What are
> you going to say to users whose code will break, and it will break
> subtly under particular optimization/code shape scenarios? That they
> should've followed the 20+ yr old JLS spec, and now that
> reachabilityFence is out, they should sweep all their code and their
> 3rd party deps to ensure these subtle lifetimes issues are fixed?

Yes, more or less.  It's happened in the past, and it'll continue
to happen.  To begin with people didn't much have to worry about
subtleties to do with finalization and lifetimes, and were bitten
by it, and fixed their code.

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

Justin Sampson
In reply to this post by Andrew Haley
Andrew Haley wrote:

> It's not just HotSpot, though: some VMs are even more aggressive,
> and we have seen finalizers executed even before constructors have
> completed. And that is allowed by the specification.

How so? I mean, I understand that an object may become unreachable
before the completion of its constructor due to inlining, but the
finalizer is not allowed to run at that point:

"The completion of an object's constructor happens-before the
execution of its finalize method (in the formal sense of
happens-before)."

https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.6

Cheers,
Justin

_______________________________________________
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

Justin Sampson
In reply to this post by Vitaly Davidovich
Vitaly Davidovich wrote:

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

Is the try/finally necessary? How about instead:

public ByteBuffer put(int i, byte x) {
    unsafe.putByte(ix(checkIndex(i)), x);
    Fences.reachabilityFence(this);
    return this;
}

public int get(int i) {
    int result = unsafe.getByte(ix(checkIndex(i)));
    Fences.reachabilityFence(this);
    return result;
}

Cheers,
Justin

_______________________________________________
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
I don't think it's needed for correctness in these examples, but I can see people adopting try/finally forms as a way to "outline" the lifetime extension and not mix it in with the core logic.  This also helps in ensuring code refactoring does not inadvertently place code that needs lifetime extension after reachabilityFence by mistake.

On Tue, Dec 8, 2015 at 10:40 PM, Justin Sampson <[hidden email]> wrote:
Vitaly Davidovich wrote:

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

Is the try/finally necessary? How about instead:

public ByteBuffer put(int i, byte x) {
    unsafe.putByte(ix(checkIndex(i)), x);
    Fences.reachabilityFence(this);
    return this;
}

public int get(int i) {
    int result = unsafe.getByte(ix(checkIndex(i)));
    Fences.reachabilityFence(this);
    return result;
}

Cheers,
Justin


_______________________________________________
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

Peter Levart
In reply to this post by Justin Sampson


On 12/09/2015 04:25 AM, Justin Sampson wrote:

> Andrew Haley wrote:
>
>> It's not just HotSpot, though: some VMs are even more aggressive,
>> and we have seen finalizers executed even before constructors have
>> completed. And that is allowed by the specification.
> How so? I mean, I understand that an object may become unreachable
> before the completion of its constructor due to inlining, but the
> finalizer is not allowed to run at that point:
>
> "The completion of an object's constructor happens-before the
> execution of its finalize method (in the formal sense of
> happens-before)."
>
> https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-12.6
>
> Cheers,
> Justin

A little later down in "12.6.1. Implementing Finalization" JLS also writes:

"An object o is not finalizable until its constructor has invoked the
constructor for Object on o and that invocation has completed
successfully (that is, without throwing an exception)."

So we have two seemingly contradictive definitions using: object's
constructor vs. Object's constructor

I think the definition that uses "object's constructor" is not precise.
A general object may have many costructors which are invoked in chain.
While java.lang.Object only has a single constructor. So I would trust
the later definition. Besides, this is how it is implemented in Hotspot/JDK.

Regards, Peter

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

David Holmes-6
JLS 12.6.1 establishes a minimum requirement for an object to be finalizable
- it ensures that the object has the minimalist form for a valid object: it
has been allocated and Object's constructor has succeeded. If something goes
wrong prior to that point then the object can not be finalized.

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.

David

> -----Original Message-----
> From: [hidden email] [mailto:concurrency-
> [hidden email]] On Behalf Of Peter Levart
> Sent: Wednesday, December 9, 2015 7:03 PM
> To: Justin Sampson; Andrew Haley; Vitaly Davidovich
> Cc: [hidden email]
> Subject: Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
>
>
> On 12/09/2015 04:25 AM, Justin Sampson wrote:
> > Andrew Haley wrote:
> >
> >> It's not just HotSpot, though: some VMs are even more aggressive,
> >> and we have seen finalizers executed even before constructors have
> >> completed. And that is allowed by the specification.
> > How so? I mean, I understand that an object may become unreachable
> > before the completion of its constructor due to inlining, but the
> > finalizer is not allowed to run at that point:
> >
> > "The completion of an object's constructor happens-before the
> > execution of its finalize method (in the formal sense of
> > happens-before)."
> >
> > https://docs.oracle.com/javase/specs/jls/se8/html/jls-12.html#jls-
> 12.6
> >
> > Cheers,
> > Justin
>
> A little later down in "12.6.1. Implementing Finalization" JLS also
> writes:
>
> "An object o is not finalizable until its constructor has invoked the
> constructor for Object on o and that invocation has completed
> successfully (that is, without throwing an exception)."
>
> So we have two seemingly contradictive definitions using: object's
> constructor vs. Object's constructor
>
> I think the definition that uses "object's constructor" is not precise.
> A general object may have many costructors which are invoked in chain.
> While java.lang.Object only has a single constructor. So I would trust
> the later definition. Besides, this is how it is implemented in
> Hotspot/JDK.
>
> Regards, Peter
>
> >
> > _______________________________________________
> > 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
Reply | Threaded
Open this post in threaded view
|

Re: DirectByteBuffers and reachabilityFence

Alexandre De Champeaux
In reply to this post by Vitaly Davidovich
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
12345 ... 8