No vacancy at the Roach Motel for Thread.holdsLock?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

No vacancy at the Roach Motel for Thread.holdsLock?

Jason Mehrens
Hello c-i list,

Per http://jeremymanson.blogspot.com/2007/05/roach-motels-and-java-memory-model.html, the compiler is allowed to move code into synchronized blocks.
So given the following code:

===============
    private final Object toStringLock = new Object();

    @Override
    public String toString() {
        if (!Thread.holdsLock(toStringLock)) {
            synchronized (toStringLock) {
                return deepToString();
            }
        } else {
            return "self";
        }
    }
==============

What prevents the compiler from moving Thread.holdsLock into the synchronized block and changing the intent?  
My assumption is that there must be a happens-before  edge in holdsLock that I'm failing to identify when browsing the openjdk source.

Thanks,

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

Re: No vacancy at the Roach Motel for Thread.holdsLock?

Vitaly Davidovich
holdsLock() is a native call, which lands at http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/7c1c2a79f981/src/share/vm/prims/jvm.cpp#l3155.  That in turn proceeds to http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/file/7c1c2a79f981/src/share/vm/runtime/synchronizer.cpp#l782, where the core logic is implemented.

holdsLock() being a native call which does not appear to be a compiler intrinsic means the compiler will not be able to reason about moving this call with respect to other operations.  Probably not an overly satisfactory answer for some folks though ...

On Wed, Jan 13, 2016 at 10:53 AM, Jason Mehrens <[hidden email]> wrote:
Hello c-i list,

Per http://jeremymanson.blogspot.com/2007/05/roach-motels-and-java-memory-model.html, the compiler is allowed to move code into synchronized blocks.
So given the following code:

===============
    private final Object toStringLock = new Object();

    @Override
    public String toString() {
        if (!Thread.holdsLock(toStringLock)) {
            synchronized (toStringLock) {
                return deepToString();
            }
        } else {
            return "self";
        }
    }
==============

What prevents the compiler from moving Thread.holdsLock into the synchronized block and changing the intent?
My assumption is that there must be a happens-before  edge in holdsLock that I'm failing to identify when browsing the openjdk source.

Thanks,

Jason
_______________________________________________
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: No vacancy at the Roach Motel for Thread.holdsLock?

Aleksey Shipilev-2
In reply to this post by Jason Mehrens
On 01/13/2016 06:53 PM, Jason Mehrens wrote:
> Per
> http://jeremymanson.blogspot.com/2007/05/roach-motels-and-java-memory-model.html,
> the compiler is allowed to move code into synchronized blocks.

"Roach Motel" is not a generic rule that allows pulling arbitrary code
into the synchronized blocks. Its scope is mandated by what JMM and
general semantic rules allow.

The essence of "Roach Motel" interpretation is that the program behavior
after *some* transformations that effectively "pull in" the code into
sync block is still a subset of the behaviors of original program, and
thus a modification is sound.

> So given the following code:
>
> ===============
>     private final Object toStringLock = new Object();
>
>     @Override
>     public String toString() {
>         if (!Thread.holdsLock(toStringLock)) {
>             synchronized (toStringLock) {
>                 return deepToString();
>             }
>         } else {
>             return "self";
>         }
>     }
> ==============
>
> What prevents the compiler from moving Thread.holdsLock into the
> synchronized block and changing the intent?
Pragmatically, two things:

 a) Thread.hasLock is native, and so the side-effects are opaque in it.
It takes an insane compiler to move the code around without
understanding what that code does.

Let's take another example though:

===============
     private final Object toStringLock = new Object();
     private boolean isAFullMoonToday;

     @Override
     public String toString() {
         if (isAFullMoonToday) { // some arbitrary predicate
             synchronized (toStringLock) {
                 return deepToString();
             }
         } else {
             return "self";
         }
     }
 ==============

 b) Moving isAFullMoonToday into the synchronized section would mean the
code *always* locks. In other words, the compiler would introduce
locking on the path that did not have any locks before -- a recipe for
deadlock. In yet another words, messing with control flow that changes
the locking order is a no-no for a compiler.

Thanks,
-Aleksey


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

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

Re: No vacancy at the Roach Motel for Thread.holdsLock?

thurstonn
I'm not sure the "blueMoon" code is analogous (since it's a POSV and may be cached in a register, even ellided)

Although not directly related to roach motel, the OP example seems similar to the discussion related to the memory semantics of unpark/park  (another native method) which we had here about a year ago, and ultimately was left unresolved (and is fundamental to a lot of code in j.u.c).

I suspect there are many more such examples, where there are native methods with unspecified memory semantics that are being used (can only usefully be used?) as if they have acquire or release semantics.

It's a problem, I think
Reply | Threaded
Open this post in threaded view
|

Re: No vacancy at the Roach Motel for Thread.holdsLock?

Alex Otenko
In reply to this post by Aleksey Shipilev-2
The problem with this example is not the compiler making good or bad decisions, but someone making decisions whether to lock based on values that aren’t part of synchronization order.

Alex


> On 13 Jan 2016, at 16:48, Aleksey Shipilev <[hidden email]> wrote:
>
> On 01/13/2016 06:53 PM, Jason Mehrens wrote:
>> Per
>> http://jeremymanson.blogspot.com/2007/05/roach-motels-and-java-memory-model.html,
>> the compiler is allowed to move code into synchronized blocks.
>
> "Roach Motel" is not a generic rule that allows pulling arbitrary code
> into the synchronized blocks. Its scope is mandated by what JMM and
> general semantic rules allow.
>
> The essence of "Roach Motel" interpretation is that the program behavior
> after *some* transformations that effectively "pull in" the code into
> sync block is still a subset of the behaviors of original program, and
> thus a modification is sound.
>
>> So given the following code:
>>
>> ===============
>>    private final Object toStringLock = new Object();
>>
>>    @Override
>>    public String toString() {
>>        if (!Thread.holdsLock(toStringLock)) {
>>            synchronized (toStringLock) {
>>                return deepToString();
>>            }
>>        } else {
>>            return "self";
>>        }
>>    }
>> ==============
>>
>> What prevents the compiler from moving Thread.holdsLock into the
>> synchronized block and changing the intent?
>
> Pragmatically, two things:
>
> a) Thread.hasLock is native, and so the side-effects are opaque in it.
> It takes an insane compiler to move the code around without
> understanding what that code does.
>
> Let's take another example though:
>
> ===============
>     private final Object toStringLock = new Object();
>     private boolean isAFullMoonToday;
>
>     @Override
>     public String toString() {
>         if (isAFullMoonToday) { // some arbitrary predicate
>             synchronized (toStringLock) {
>                 return deepToString();
>             }
>         } else {
>             return "self";
>         }
>     }
> ==============
>
> b) Moving isAFullMoonToday into the synchronized section would mean the
> code *always* locks. In other words, the compiler would introduce
> locking on the path that did not have any locks before -- a recipe for
> deadlock. In yet another words, messing with control flow that changes
> the locking order is a no-no for a compiler.
>
> Thanks,
> -Aleksey
>
> _______________________________________________
> 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