20 ms·

Regarding the question in the 1st topic: (https://shipilev.net/jvm-anatomy-park/1-lock-coarsening-for-loops/ https://shipilev.net/jvm-anatomy-park/1-lock-coarse

Regarding the question in the 1st topic: (https://shipilev.net/jvm-anatomy-park/1-lock-coarsening-for-loops/ https://shipilev.net/jvm-anatomy-park/1-lock-coarsening-for-...) " for (...) { synchronized (obj) { // something } } …could it optimize into this? synchronized (this) { for (...) { <----XXX---- // something } } " The answer would be no in general, I think, since it is unsafe. Moving the entire loop into the synchronized block would make the statement marked XXX above, i.e. "for (...)" execute inside the synchronized block, which has the potential to change the semantics (for example, the statement may include an rpc, and we don't want to make that rpc under the lock). The earlier optimization of synchronized (this) { statement1; } synchronized (this) { statement2; } to synchronized (this) { statement1; statement2; } is safe.

chrisseaton 9y ago
> Moving the entire loop into the synchronized block would make the statement marked XXX above, i.e. "for (...)" execute inside the synchronized block, which has the potential to change the semantics (for example, the statement may include an rpc, and we don't want to make that rpc under the lock). That's not change in semantics. The JVM doesn't provide any guarantees about parallelism or scheduling semantics in the first place so how can you possibly argue about the semantics of holding a lock too long? And it's unsafe? For what definition of safety? But in practice - yes you obviously wouldn't do this if the code in the loop control code had any side effects.
- zanker_swe 9y ago
  >And it's unsafe? For what definition of safety? Let me try to answer with an example. Let us say, we have original code like this: synchronized(this) { a(); b(); } c(); synchronized(this) { d(); e(); } It would be unsafe (in general) to transform the above code to synchronized(this) { a(); b(); c(); d(); e(); } Simply because Compiler does not know (again, in general) what may happen during the execution of c(). However, the following transformation is safe (the timing behavior changes, but as you point out, that is not a guarantee programmers should expect). synchronized(this) { p(); q(); } synchronized(this) { r(); s(); } to synchronized(this) { p(); q(); r(); s(); } since there is nothing happening between the two synchronized sections. For the for loop, an example of code where pulling the synchronized statement out of the loop is problematic: for(a = AcquireLock(), c = 0; c < 100; c++) { synchronized(this) { f(); } } ReleaseLock(a); I don't think it is safe to transform to a = AcquireLock(); synchronized(this) { for(c = 0; c < 100; c++) { f(); } } ReleaseLock(a); Perhaps you (and the original blog post) assume we are only talking about movement of code after ensuring that such movement is safe, but it was not clear from the document.
  - chrisseaton 9y ago
    Just looking at your final example, I can't understand how you can say that is unsafe. How would anyone be able to tell the difference between the two? I think anything you tell me I'm just going to be able to answer 'but Java never guaranteed you that in the first place'. If nobody can tell the difference then how can it be unsafe?
    - zanker_swe 9y ago
      >Just looking at your final example, I can't understand how you can say that is unsafe Let me change the example a bit. Say we have two locks aL and bL, that we must always acquire in the order aL first and then bL. Following the rule, say we write code like this: import java.util.concurrent.locks.ReentrantLock; class X { private static ReentrantLock aL = new ReentrantLock(); private static ReentrantLock bL = new ReentrantLock(); static int x = 0; static int c = 0; static public void main(String[] args) { for(aL.lock(); c < 100; c++) { synchronized(bL) { x = x + 0x42; } } aL.unlock(); } } If I understood it right, the blog post was asking a question whether JVM can transform this to: import java.util.concurrent.locks.ReentrantLock; class X { private static ReentrantLock aL = new ReentrantLock(); private static ReentrantLock bL = new ReentrantLock(); static int x = 0; static int c = 0; static public void main(String[] args) { synchronized(bL) { for(aL.lock(); c < 100; c++) { x = x + 0x42; } aL.unlock(); } // end synnchronized } } Since the locks are now acquired in a different order, does that not qualify as observable behavior?
      - chrisseaton 9y ago
        But that's just a different example to the one you gave before. In your previous example acquiring the explicit lock always came before the start of synchronised block, both before and after the rewrite. You've changed it here so it's a different question.
        zanker_swe 9y ago
        Sorry, I meant to write: synchronized(this) { a = AcquireLock(); for(c = 0; c < 100; c++) { f(); } } ReleaseLock(a); which is inline with what the blog post was proposing. To repeat the blog is a question: for (...) { synchronized (obj) { // something } } …could it optimize into this? synchronized (this) { for (...) { // something } } My answer to that is in general, no.
thomaslee 9y ago
> The answer would be no in general, I think, since it is unsafe. > ... > for example, the statement may include an rpc, and we don't want to make that rpc under the lock I do agree that it's not a "safe" optimization in the extreme general case (so don't go rewriting your code assuming it's equivalent!), but in the case where the loop is a candidate for unrolling it works just fine. Imagine you had a more CPU- or memory-bound workload, and these benchmarks are a whole lot more interesting. Put another way: if there's an RPC call in the for loop, the time spent in the RPC will dwarf the work involved in executing loop itself so ... odds are good it's not going to be a candidate for unrolling anyways. :)
endoplasm 9y ago
Neither of those optimizations looks safe to me. Couldn't they potentially introduce deadlocks?
- adrianmonk 9y ago
  I had the same reaction, but now I'm starting to suspect it actually is safe, for a reason that's not immediately obvious. Consider the unoptimized version. It's possible that after you release the lock, another thread will take it. But I'm pretty confident there is no guarantee of this, and your own thread might immediately take the lock back. (Probably often, especially if there's just one CPU.) If taking the lock back immediately causes a deadlock, then your code is already broken as written. So you already must account for the possibility that nothing changes between the time you release the lock and take it again. All the optimization does is take away the _other_ possibility, which was something you couldn't rely on anyway.
  - endoplasm 9y ago
    Yes, you're right. The definition of an optimization being ``safe'' is tricky in the presence of non-determinism. The set of possible executions of a correctly optimized program should be a subset of---and specifically, need not be necessarily equal to---the possible executions of the original unoptimized version.

chrisseaton 9y ago

> Moving the entire loop into the synchronized block would make the statement marked XXX above, i.e. "for (...)" execute inside the synchronized block, which has the potential to change the semantics (for example, the statement may include an rpc, and we don't want to make that rpc under the lock). That's not change in semantics. The JVM doesn't provide any guarantees about parallelism or scheduling semantics in the first place so how can you possibly argue about the semantics of holding a lock too long? And it's unsafe? For what definition of safety? But in practice - yes you obviously wouldn't do this if the code in the loop control code had any side effects.

zanker_swe 9y ago
>And it's unsafe? For what definition of safety? Let me try to answer with an example. Let us say, we have original code like this: synchronized(this) { a(); b(); } c(); synchronized(this) { d(); e(); } It would be unsafe (in general) to transform the above code to synchronized(this) { a(); b(); c(); d(); e(); } Simply because Compiler does not know (again, in general) what may happen during the execution of c(). However, the following transformation is safe (the timing behavior changes, but as you point out, that is not a guarantee programmers should expect). synchronized(this) { p(); q(); } synchronized(this) { r(); s(); } to synchronized(this) { p(); q(); r(); s(); } since there is nothing happening between the two synchronized sections. For the for loop, an example of code where pulling the synchronized statement out of the loop is problematic: for(a = AcquireLock(), c = 0; c < 100; c++) { synchronized(this) { f(); } } ReleaseLock(a); I don't think it is safe to transform to a = AcquireLock(); synchronized(this) { for(c = 0; c < 100; c++) { f(); } } ReleaseLock(a); Perhaps you (and the original blog post) assume we are only talking about movement of code after ensuring that such movement is safe, but it was not clear from the document.
- chrisseaton 9y ago
  Just looking at your final example, I can't understand how you can say that is unsafe. How would anyone be able to tell the difference between the two? I think anything you tell me I'm just going to be able to answer 'but Java never guaranteed you that in the first place'. If nobody can tell the difference then how can it be unsafe?
  - zanker_swe 9y ago
    >Just looking at your final example, I can't understand how you can say that is unsafe Let me change the example a bit. Say we have two locks aL and bL, that we must always acquire in the order aL first and then bL. Following the rule, say we write code like this: import java.util.concurrent.locks.ReentrantLock; class X { private static ReentrantLock aL = new ReentrantLock(); private static ReentrantLock bL = new ReentrantLock(); static int x = 0; static int c = 0; static public void main(String[] args) { for(aL.lock(); c < 100; c++) { synchronized(bL) { x = x + 0x42; } } aL.unlock(); } } If I understood it right, the blog post was asking a question whether JVM can transform this to: import java.util.concurrent.locks.ReentrantLock; class X { private static ReentrantLock aL = new ReentrantLock(); private static ReentrantLock bL = new ReentrantLock(); static int x = 0; static int c = 0; static public void main(String[] args) { synchronized(bL) { for(aL.lock(); c < 100; c++) { x = x + 0x42; } aL.unlock(); } // end synnchronized } } Since the locks are now acquired in a different order, does that not qualify as observable behavior?
    - chrisseaton 9y ago
      But that's just a different example to the one you gave before. In your previous example acquiring the explicit lock always came before the start of synchronised block, both before and after the rewrite. You've changed it here so it's a different question.
      - zanker_swe 9y ago
        Sorry, I meant to write: synchronized(this) { a = AcquireLock(); for(c = 0; c < 100; c++) { f(); } } ReleaseLock(a); which is inline with what the blog post was proposing. To repeat the blog is a question: for (...) { synchronized (obj) { // something } } …could it optimize into this? synchronized (this) { for (...) { // something } } My answer to that is in general, no.

thomaslee 9y ago

> The answer would be no in general, I think, since it is unsafe. > ... > for example, the statement may include an rpc, and we don't want to make that rpc under the lock I do agree that it's not a "safe" optimization in the extreme general case (so don't go rewriting your code assuming it's equivalent!), but in the case where the loop is a candidate for unrolling it works just fine. Imagine you had a more CPU- or memory-bound workload, and these benchmarks are a whole lot more interesting. Put another way: if there's an RPC call in the for loop, the time spent in the RPC will dwarf the work involved in executing loop itself so ... odds are good it's not going to be a candidate for unrolling anyways. :)

endoplasm 9y ago

Neither of those optimizations looks safe to me. Couldn't they potentially introduce deadlocks?

adrianmonk 9y ago
I had the same reaction, but now I'm starting to suspect it actually is safe, for a reason that's not immediately obvious. Consider the unoptimized version. It's possible that after you release the lock, another thread will take it. But I'm pretty confident there is no guarantee of this, and your own thread might immediately take the lock back. (Probably often, especially if there's just one CPU.) If taking the lock back immediately causes a deadlock, then your code is already broken as written. So you already must account for the possibility that nothing changes between the time you release the lock and take it again. All the optimization does is take away the _other_ possibility, which was something you couldn't rely on anyway.
- endoplasm 9y ago
  Yes, you're right. The definition of an optimization being ``safe'' is tricky in the presence of non-determinism. The set of possible executions of a correctly optimized program should be a subset of---and specifically, need not be necessarily equal to---the possible executions of the original unoptimized version.