[concurrency-interest] DirectByteBuffers and reachabilityFence

Viktor Klang viktor.klang at gmail.com
Tue Dec 15 03:36:51 EST 2015


wow, nasty indeed!

-- 
Cheers,
√
On 15 Dec 2015 05:01, "David Holmes" <davidcholmes at aapt.net.au> wrote:

> Here’s another nasty little reachability/finalize example:
>
>
>
> https://bugs.openjdk.java.net/browse/JDK-8145304
>
>
>
> David
>
>
>
> *From:* concurrency-interest-bounces at cs.oswego.edu [mailto:
> concurrency-interest-bounces at cs.oswego.edu] *On Behalf Of *Gil Tene
> *Sent:* Tuesday, December 15, 2015 3:48 AM
> *To:* Timo Kinnunen
> *Cc:* Doug Lea; concurrency-interest at cs.oswego.edu
> *Subject:* Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
>
>
>
>
> On Dec 14, 2015, at 4:27 AM, Timo Kinnunen <timo.kinnunen at gmail.com>
> wrote:
>
>
>
> Re: “If the caller doesn't use the return value (which is a common case
> for calling get()), and the caller inlined get(), then that return
> statement doesn't exist as far as the JIT is concerned; it is dead code
> that can (and does) get legitimately be removed without breaking any rules.
> Then 'this' goes out of scope before the unsafe access, and boom.“
>
>
>
> This is the case when I do such refactorings in source code using an IDE.
> It doesn’t seem like the compiler allows the transforms it does at runtime
> to affect the reachability of the objects involved, however. The biggest
> difference is that whether the return value is read or not isn’t what
> decides if the instance gets collected before the method has returned.
>
>
>
> Lack of success in making the object reachability race happen doesn't
> equate to the race not being there… The compiler will absolutely shorten
> the reachability (from a frame) of anything that is no longer used in the
> frame and is does not escape it. It does so by no longer describing it in
> the oopmaps. If that object is not otherwise reachable, it is susceptible
> to having loss-of-reachability actions (such as queueing references or
> running finalizers) occur on it in the middle of the frame's execution.
> It's not something you can detect directly by reading the generated code
> (you'd have to examine the oopmaps). And it's not something that you can to
> disprove with a test.
>
>
>
>
>
> These are factors that I see that do play a part in the decision:
>
> 1)      Is the receiver not being used baked into the bytecode or can it
> be affected by evaluating something at runtime.
>
> 2)      Is the instance method a leaf method or has it called another
> instance method on the receiver.
>
>
>
> It is helpful to imagine "infinite inlining" when thinking about these
> things, and look at the code as one big flat frame. Then see how far in
> that frame a reference is still used (including being passed to later
> calls). Bytecodes and method boundaries don't really mean anything for this
> reachability issue.
>
>
>
>
>
> My earlier theory doesn’t predict the second point and I wonder if the
> compiler is being overly conservative or if there’s something more to it.
>
>
>
> This is the caller:
>
>
>
>                 public void work() throws InterruptedException {
>
>                                 int which = this.whichRun;
>
>                                 report(this.letsDoThisThing(), which);
>
>                                 report(DETAIL, which);
>
>                                 reachabilityEnds(which);
>
>                 }
>
>
>
> And this is the instance method that I’m observing:
>
>
>
>                 private BrokenFinalizeExample letsDoThisThing() throws
> InterruptedException {
>
>                                 report("Work started", this.whichRun);
>
>                                 collect(this.whichRun);
>
>                                 Thread.sleep(WAIT_MILLIS);
>
>                                 if(CONDITIONALLY_COMPILED) return
> EFFECTIVELY_STATIC ? null : this;
>
>                                 return new Boolean(EFFECTIVELY_STATIC) ?
> null : this;
>
>                 }
>
>
> It’s only when CONDITIONALLY_COMPILED and EFFECTIVELY_STATIC are both true
> (and compile-time constants) and not if the caller uses the return value
> that the receiver gets collected before the sleep.
>
>
>
> When your compile time constants are not set, this example can't shorten
> the reachability of this in letsDoThisThing(), or in work() even when it
> inlines it, because 'this' can make it to the subsequent call to report().
>
>
>
> Try the same without passing the return value to report(), and see if you
> can trigger things in the sleep...
>
>
>
> This suggests that whether DBB.get() is safe or not can be determined
> without examining its callers, which is how it should be.
>
>
>
> This is not something you can deduce from this test code, since the caller
> in the test code consumes the return value from the caller and uses it in
> report().
>
>
>
> Determining that DBB.get() "isn't safe" doesn't require examining the
> callers. It can be shown to be "not-safe by the mere possibility of an
> inlining caller hat doesn't make actual (required to occur after the call)
> use of the return value, making 'this' potentially become unreachable
> before the internally tracked ID (the off heap buffer address in this case)
> is used to access a resource. That possibility both exists and is probably
> a common case.
>
>
>
>
>
>
>
> Observed with  “-Xcomp -XX:+TieredCompilation” settings.
>
>
>
> And the positive trace:
>
>
>
>           2.243455 ms: Run 1
>
>         382.288398 ms: Work started 1
>
>         386.992615 ms: GC 1
>
>         440.442752 ms: Finalize 1
>
>        2433.652559 ms: null 1
>
>        2433.809728 ms: Unconditional 'instance method' ends 1
>
>        2433.887848 ms: GC 1
>
>        2445.546556 ms: Reachability definitely ends 1
>
>        2445.671485 ms: GC 1
>
>        2457.054916 ms: Run 2
>
>        2461.741153 ms: Work started 2
>
>        2462.251409 ms: GC 2
>
>        2473.351502 ms: Finalize 2
>
>        4473.494721 ms: null 2
>
>        4473.611900 ms: Unconditional 'instance method' ends 2
>
>        4473.644760 ms: GC 2
>
>        4480.421312 ms: Reachability definitely ends 2
>
>        4480.504082 ms: GC 2
>
>
>
> --
> Have a nice day,
> Timo
>
> Sent from Mail for Windows 10
>
>
>
>
>
>
> *From: *Gil Tene
> *Sent: *Sunday, December 13, 2015 03:11
> *To: *Timo Kinnunen
> *Cc: *Vitaly Davidovich;Peter;Doug Lea;concurrency-interest at cs.oswego.edu
> *Subject: *Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
>
>
>
>
>
>
> Sent from Gil's iPhone
>
>
> On Dec 12, 2015, at 12:38 AM, Timo Kinnunen <timo.kinnunen at gmail.com>
> wrote:
>
> There’s many ways DBB.get() could be written and many ways a compiler
> could transform it when inlining it inside a calling method. The
> implementation that I looked at returns the receiver as the method call
> completes. My position is that within such a method, the range where the
> receiver is reachable is already as large as it can ever be and inserting a
> reachabilityFence can’t and won’t make it any larger. If it didn’t return
> the receiver the situation would be completely different.
>
>
>
> If the caller doesn't use the return value (which is a common case for
> calling get()), and the caller inlined get(), then that return statement
> doesn't exist as far as the JIT is concerned; it is dead code that can (and
> does) get legitimately be removed without breaking any rules. Then 'this'
> goes out of scope before the unsafe access, and boom.
>
>
>
> put() provides the same fun adventures in accessing potentially-freed
> memory, and probably a more readable example of the race.
>
>
>
>
> I can easily observe the receiver getting garbage collected in under a
> second while an instance method is executing when that method returns a
> null value that no-one reads. If it returns the receiver – still with
> no-one to read it -- then I can’t make it get finalized before it returns.
> Not even if I make it extremely unlikely to actually return the receiver
> and not a null, like this:
>
>                 return ThreadLocalRandom.current().nextInt(100) == 101 ?
> this : null;
>
>
>
> Inlining eliminates the return.
>
>
>
>
>
>
> As for the theory side of the issue, my position there is that asking what
> sort of data races thread t1 could observe in the presence of some garbage
> collection thread GC1 is fruitless, because garbage collection isn’t a
> synchronization action, the JMM doesn’t recognize that these threads
> synchronize with each other and in any case if thread T1 won’t attempt to
> synchronize with the possibly-non-existent GC1 then the whole of JLS 17.4
> doesn’t even apply.
>
>
>
> There are no data races between Java threads and GC threads. But there
> certainly are data races between Java thread t1 and another Java thread t2
> that is executing code in response to a phantom/weak/soft red being
> enqueued (as a result of GC activity on a reference that has become
> unreachable), or a Java thread t3 that is executing a finalizer (finalizers
> are executed in Java threads, not in collector threads).
>
>
>
> I think what ultimately decides the case at least for those instance
> methods that return their receiver is this part in 12.6.1: “Furthermore,
> none of the pre-finalization reads of fields of that object may see writes
> that occur after finalization of that object is initiated.” For our
> purposes reading the receiver to pass it to the caller could be construed
> as a read of all of the receiver’s fields .
>
>
>
> First, The finalization parts of the spec are irrelevant to DBB, since it
> doesn't do finalization. The races involved effect phantom/weak/soft refs
> just as much as they  do finalization.
>
>
>
> But even when looking at finalizers (the Cleaner work DBB does could be
> similarly done with a finalizer), there are no post-finalization field
> reads or writes involved in the DBB races for get() and put(): A local
> method variable value (a long representing an off heap address) is
> determined by reading ('pre-fnalzation') a field of 'this'; Then 'this'
> becomes unreachable. Then a cleaner executes because a phantom ref to the
> now unreachable 'this' got enqueued; Then the native memory gets freed;
> (then lots of interesting things, like that memory being allocated for some
> other purpose, can occur); Then ('post finalization') the unsafe access to
> that native memory occurs using the method local variable.
>
>
>
>
>
>
>
>
>
>
>
>
>
>
>
> --
> Have a nice day,
> Timo
>
> Sent from Mail for Windows 10
>
>
>
>
>
>
> *From: *Vitaly Davidovich
> *Sent: *Friday, December 11, 2015 22:51
> *To: *Timo Kinnunen
> *Cc: *Gil Tene;Peter;Doug Lea;concurrency-interest at cs.oswego.edu
> *Subject: *Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
>
>
>
>
> So why is the DBB.get() method safe and not a potentially asynchronous
> destructor? Simply because as the last thing it does is a “return this;”
> there is no place inside it where a reachabilityFence(this) could be placed
> such that the receiver would not be used afterwards! This doesn’t by itself
> mean that every call to it is done in a safe manner but it does mean that a
> reachability fence can’t fix the implementation of DBB.get() because
> DBB.get() isn’t broken.
>
>
>
> It's been shown a few times already in this thread why DBB.get() is
> actually broken, but happens to be lucky (presumably).
>
>
>
> I think this is still safe, though, as everything until here has been
> limited to a single thread, therefore correctly synchronized, therefore
> data-race free and therefore sequentially consistent. If a single-threaded
> program could observe a crash here then it would mean the GC had observed
> the return of a method call before the method call returned, which would
> mean the thread’s execution wasn’t sequentially consistent, which is a
> contradiction. Therefore such a reordering is not a legal reordering.
>
>
>
> There's no contradiction, really.  The GC does not "observe return of a
> method", the GC observes oopmaps the compiler gives it; these oopmaps
> describe which oops are live at the point of GC (safepoint).  If compiler
> determines that `this` is not live anymore, GC will pick up on that.  The
> problem is simply that JIT thinks it's dealing with plain "long" values,
> but in reality that's a native pointer.  This is just an impedance mismatch
> between managed and native memory playing together.
>
>
>
>
>
> On Fri, Dec 11, 2015 at 3:56 PM, Timo Kinnunen <timo.kinnunen at gmail.com>
> wrote:
>
> Hi,
>
>
>
> I think I’ve figured out why the implementation of
> DirectByteBuffer.get(byte[], int, int) is safe.
>
>
>
> First, please convince yourself that reachabilityFence(this) is
> semantically equivalent or at least as-if compatible with a
> reachabilityPotentiallyEnds(this) call, with the method
> reachabilityPotentiallyEnds() being where all the finalization magic is
> implemented. (This is just for illustration and not the main point.)
>
>
>
> Convinced already? Good ��
>
>
>
> First, consider a constructor. The execution of a constructor starts in a
> static context, where pretty much nothing except evaluating arguments and
> calling another constructor of the same class or some superclass
> constructor is allowed. After that call completes normally, the context
> becomes that of the newly constructed instance, all restrictions of the
> static context are lifted and the rest of the constructor proceeds to
> execute as usual.
>
>
>
> Now consider an instance method where the receiver is not used beyond a
> certain point, its bytecode with slot 0 not read after that point, and the
> native code version which therefore can overwrite and reuse the register
> housing the this-pointer as needed. Such a method could be refactored into
> methods A and B, where method A executes before B and where the receiver is
> used in A but not in B, so that method B can be static. For such a method
> an implicit reachabilityPotentiallyEnds(this) call could be considered to
> exist between the parts A and B. If the receiver does become unreachable in
> such a method then the result of such an execution would be a dual or the
> inverse to a constructor call. Let’s call such methods potentially
> asynchronous destructors:
>
>
>
> A Potentially Asynchronous Destructor is an instance method where it’s
> possible to place a reachabilityFence(this) marker in such a way that the
> receiver is not referenced after execution reaches the marker and the rest
> of the execution happens in an effectively static context.
>
>
>
> Given this definition, any method where an explicit
> reachabilityFence(this) needs to be inserted so that the boundary between A
> and B moves to some later position to guarantee its safety is a potentially
> asynchronous destructor.
>
>
>
> So why is the DBB.get() method safe and not a potentially asynchronous
> destructor? Simply because as the last thing it does is a “return this;”
> there is no place inside it where a reachabilityFence(this) could be placed
> such that the receiver would not be used afterwards! This doesn’t by itself
> mean that every call to it is done in a safe manner but it does mean that a
> reachability fence can’t fix the implementation of DBB.get() because
> DBB.get() isn’t broken.
>
>
>
> I think this is still safe, though, as everything until here has been
> limited to a single thread, therefore correctly synchronized, therefore
> data-race free and therefore sequentially consistent. If a single-threaded
> program could observe a crash here then it would mean the GC had observed
> the return of a method call before the method call returned, which would
> mean the thread’s execution wasn’t sequentially consistent, which is a
> contradiction. Therefore such a reordering is not a legal reordering.
>
> So, what do you think? Sound reasoning or not?
>
>
>
>
> --
> Have a nice day,
> Timo
>
> Sent from Mail for Windows 10
>
>
>
>
>
>
> *From: *Gil Tene
> *Sent: *Friday, December 11, 2015 06:01
> *To: *Peter
> *Cc: *Doug Lea;concurrency-interest at cs.oswego.edu
>
>
> *Subject: *Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
>
>
>
>
>
>
> > On Dec 10, 2015, at 6:47 PM, Peter <jini at zeus.net.au> wrote:
>
> >
>
> > Would another workaround be to use a phantom reference  for the DBB?
>
>
>
> That's exactly what sun.misc.Cleaner already does, and DBB already uses a
> cleaner. Cleaners and phantom references (or weak, or soft, or finalizers)
> do not get rid of the race. A DBB 'this' can become unreachable between
> computing an address and accessing it, GC picks up on that and enqueues the
> phantom reference (the Cleaner object), and the Cleaner object explicitly
> frees the buffer memory that was held by the DBB instance. Even though the
> DBB memory isn't reclaimed until the next GC, that doesn't matter, since
> the buffer has been freed [and potentially recycled elsewhere] before the
> unsafe call to perform the last read or write operation into it is made.
>
>
>
> There is currently (prior to reachabilityFence) no way I know of in Java
> to close this race in DBB short of following each unsafe access (e.g. in
> each put or get) with some carefully crafted (and also expensive and/or
> optimization-hurting) volatile accesses. E.g. you can add an operation that
> stores 'this' to an extra volatile field initialized to point refer to
> 'this'. E.g. I think something like: ... unsafe(…); this.dummy.dummy =
> this; … might be sufficient to ensure that 'this' is reachable until after
> the unsafe call if dummy is volatile. But it will make buffer operations
> dead slow...
>
>
>
> >
>
> > The memory isn't reclaimed by gc until the phantom reference is cleared
> or becomes reachable.
>
> >
>
> > The phantom reference queue functionality could be implemented in static
> methods and fields.  Classes aren't gc'd until all classes in a ClassLoader
> and the ClassLoader itself becomes reachable.
>
> >
>
> > Just curious.
>
> >
>
> > Regards,
>
> >
>
> > Peter.
>
> >
>
> > Sent from my Samsung device.
>
> > ---- Original message ----
>
> > From: Doug Lea <dl at cs.oswego.edu>
>
> > Sent: 11/12/2015 10:14:54 am
>
> > To: concurrency-interest at cs.oswego.edu
>
> > Subject: Re: [concurrency-interest] DirectByteBuffers and
> reachabilityFence
>
> >
>
> > On 12/10/2015 07:56 AM, Andrew Haley wrote:
>
> > > On 12/10/2015 12:51 PM, Vitaly Davidovich wrote:
>
> > >> I can see a few reasons against extending `this` lifetime at this
> stage of
>
> > >> java's life, but what were the objections last time(s) this came up?
>
> > >
>
> > > Basically twofold.  Mostly efficiency, but also a reluctance to change
>
> > > long-settled language semantics.  Making changes to something as
>
> > > fundamental as this is always much tricker than people expect.  The
>
> > > great advantage of reachabilityFence is that we can do it without a
>
> > > lot of complex argument and politics.
>
> > >
>
> >
>
> > Like Andrew and others who have seen periodic protracted
>
> > discussions of this issue over the past decade or so, I'm not
>
> > too optimistic.
>
> >
>
> > But this is also one reason for considering a @Finalized annotation
>
> > for fields (not classes!), that would give essentially this
>
> > guarantee only in those cases it was requested. Compilers would
>
> > translate methods/blocks accessing @Finalized field r
>
> > (as in .. use(r); ...)  to  the equivalent of:
>
> >    try { ... use(r); ... } finally { reachabilityFence(); }
>
> >
>
> > In the mean time, users can at least arrange this manually though.
>
> > There is no reason to believe that the performance impact would
>
> > be any different in manual vs automated forms, as long as some of
>
> > us are willing to help hotspot minimize it.
>
> >
>
> > -Doug
>
> >
>
> >
>
> > _______________________________________________
>
> > Concurrency-interest mailing list
>
> > Concurrency-interest at cs.oswego.edu
>
> > http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
> >
>
> > _______________________________________________
>
> > Concurrency-interest mailing list
>
> > Concurrency-interest at cs.oswego.edu
>
> > http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
>
>
>
>
>
>
> _______________________________________________
> Concurrency-interest mailing list
> Concurrency-interest at cs.oswego.edu
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
>
> _______________________________________________
> Concurrency-interest mailing list
> Concurrency-interest at cs.oswego.edu
> http://cs.oswego.edu/mailman/listinfo/concurrency-interest
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://cs.oswego.edu/pipermail/concurrency-interest/attachments/20151215/9c7b501f/attachment-0001.html>


More information about the Concurrency-interest mailing list