Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods (original) (raw)

Peter Levart peter.levart at gmail.com
Tue Mar 4 07:01:46 UTC 2014

On 03/04/2014 05:09 AM, David Holmes wrote:

On 4/03/2014 1:45 PM, David Holmes wrote:

On 3/03/2014 10:56 PM, David M. Lloyd wrote:

Yes, that would necessarily be the contract of a Monitors class, just as it is part of the contract of Lock today. If your argument is that it shouldn't be allowed because it might be used wrong, we might as well just delete most of the JDK, ReentrantLock included, since it suffers from the exact same potential problem. The difference is that monitors have a simple API in the form of synchronized that people were in the past and would continue to be free (and recommended) to use.

We should not introduce anything that allows something that was guaranteed to be safe by the language, to become unsafe. So I can support a call for tryMonitorEnter, but not for explicit enter/exit actions. Which of course is not possible at the API level - Doh! Sorry.

Unless the API is made safe by packing it into a lambda-consuming method. For example:

public static boolean trySynchronized(Runnable action);

Regards, Peter

David

David -----

I would not call recommending monitors to be a "premature optimization". The memory overhead of ReentrantLock is far too large for many use cases, especially when it comes to fine-grained locking of large data sets. In fact I would only recommend Lock if your need for the extra behavior it provides (i.e., multiple conditions, tryLock, timed tryLock, and/or interruptible locking) or for the extra behavior provided by ReentrantLock itself (i.e., lock status introspection) outweighs the cost of its memory consumption, which so far has happened zero times in any of the many frameworks I maintain.

On the other hand, adding some of the missing functionality to a Monitors class would be pretty handy, especially if doing so doesn't create a large divergence from the existing implementation. Given that tryMonitorEnter already exists, implementing that method on a Monitors class seems fairly trivial, which already takes care of one out of the four special use cases for Locks. Interruptible locking would be my first choice for a number two, if this idea were actually officially on the table. Finally, an isMonitorHeld() method would tie in another branch of the discussion nicely and seems trivially possible as well. Any other information would be a purely It's worth noting that it's already possible for user code to implement the basic functionality using JNI methods, though in the interest of full disclosure it's worth noting that the enter/exit monitor JNI functions are forbidden by spec to interoperate with the corresponding bytecodes. FWIW using the word "monitor" instead of "lock" may help avoid confusion and "scare off" those pesky undergrads. On 03/02/2014 12:48 PM, Dr Heinz M. Kabutz wrote: Hi Dave,

"lighter" is subject to the HotSpot Profiler doing the optimizations that you expect. Problem is, once you begin manually locking / unlocking monitors, the byte codes are not recognizable as standard synchronized(monitor) {} code, thus the optimizations will also not be as good (or completely absent). At least that has been my experience when I tried it out a few years ago. So the argument that synchronized is faster than ReentrantLock, besides this being IMHO a premature optimization, cannot be directly applied to when we manually do use the monitorEnter/monitorExit methods. The reason why I personally think they are more dangerous than ReentrantLock is that we are used to synchronized() {} taking care of the unlocking for us. If we see a thread that is in the BLOCKED state (which only happens with synchronized) then we would typically assume that another thread is /currently /holding the monitor. Take the following example code: import sun.misc.*; import java.lang.reflect.*; public class MonitorMystery { public static Unsafe getUnsafe() { try { for (Field field : Unsafe.class.getDeclaredFields()) { if (Modifier.isStatic(field.getModifiers())) { if (field.getType() == Unsafe.class) { field.setAccessible(true); return (Unsafe) field.get(null); } } } throw new IllegalStateException("Unsafe field not found"); } catch (Exception e) { throw new IllegalStateException( "Could not initialize unsafe", e); } } public static void main(String... args) throws InterruptedException { Thread t = new Thread() { public void run() { getUnsafe().monitorEnter(MonitorMystery.class); } }; t.start(); Thread.sleep(100); System.out.println("Trying to synchronize"); synchronized (MonitorMystery.class) { System.out.println("Managed to synchronized"); } } } We now see that we never manage to synchronize in the main thread and in fact if you do a thread dump you will see a deadlock involving only a single thread :-) This is why it is so important to use the correct idioms. In my experience, there are very few Java programmers who get this right. Brian Goetz's book is correct, but there are other books out there that do a shocking job of mangling the idiom. Whenever possible, use synchronized() { }. If not, IMHO it would be preferable to switch to ReentrantLock. Regards Heinz -- Dr Heinz M. Kabutz (PhD CompSci) Author of "The Java(tm) Specialists' Newsletter" Oracle Java Champion 2005-2013 JavaOne Rock Star Speaker 2012 http://www.javaspecialists.eu Tel: +30 69 75 595 262 Skype: kabutz

David M. Lloyd wrote: Making the language Kindergarten-friendly at the cost of general usefulness is a mistake, IMO. And anyway there's nothing that is less safe about a Monitors class than ReentrantLock; on the other hand, monitors continue to be considerably lighter (size and (for most of the history of JUC) speed) by every measurement I've ever made. I would advise monitors over ReentrantLock 9 times out of 10 in any of our code. I just don't think your metaphors - neither of monitor methods being dangerous, nor of Java developers being infants - are really apt. On 03/02/2014 02:51 AM, Dr Heinz M. Kabutz wrote: With a curious 9 months old crawling around the house, I've just moved the sharp knives to the top draw in the kitchen - out of reach. I don't think we should be encouraging people to use monitor.tryLock() for various reasons: 1. We have a richer interface with Lock/ReentrantLock, including better Condition that allow easier timed wait idioms. 2. It is just too easy to get the idioms wrong for Lock.lock() and Lock.unlock(). Every time I show this idiom some people in the audience start arguing with me: lock.lock(); try { // do something } finally { lock.unlock(); } IMHO, this is really an edge case that might be useful to have semi-accessible at some point, but not something that should generally be done. It belongs in the same draw as the sharp knives and the ability to cause arbitrary asynchronous exceptions (which has been made more difficult to do in Java 8). Brian Goetz wrote: Except that Lock has features that are not supported by intrinsic locks (timed wait, interruptible wait.) So the Lock returned would not conform to Lock's contract, and attempting to call these methods would probably throw UOE. On 2/27/2014 6:12 AM, Stephen Colebourne wrote: On 26 February 2014 20:54, Martin Buchholz <martinrb at google.com> wrote: It does seem that being able to tell whether a java object monitor is currently locked is useful for debugging and monitoring - there should be a way to do that. Perhaps it would be useful to be able to expose a java object monitor as an instance of Lock? Lock lk = Lock.ofMonitor(object) if (lk.tryLock()) { ... } Such a method feels like it would be a useful missing link between synchronized and locks. Stephen

More information about the core-libs-dev mailing list