[concurrency-interest] DirectByteBuffers and reachabilityFence

Timo Kinnunen timo.kinnunen at gmail.com
Mon Dec 14 07:27:43 EST 2015


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.

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.

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. This suggests that whether DBB.get() is safe or not can be determined without examining its callers, which is how it should be. 

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
 
 
 


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://cs.oswego.edu/pipermail/concurrency-interest/attachments/20151214/fab4b3d5/attachment-0001.html>


More information about the Concurrency-interest mailing list