Unsafe: removing the monitorEnter/monitorExit/tryMonitorEnter methods

[David Holmes]

On 27/02/2014 6:38 PM, Peter Levart wrote:
> On 02/27/2014 08:41 AM, David Holmes wrote:
>> On 27/02/2014 5:30 PM, Peter Levart wrote:
>>> On 02/27/2014 08:29 AM, Peter Levart wrote:
>>>> On 02/26/2014 09:54 PM, Martin Buchholz wrote:
>>>>> I don't recall having added this particular wart
>>>>> to test/java/lang/ProcessBuilder/Basic.java, and history suggests that
>>>>> others did that.
>>>>>
>>>>> 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.
>>>>
>>>> Thread.holdsLock(Object) ?
>>>
>>> Ah, you meant to query from some other thread, right?
>>
>> Right - that is the usage in Basic.java but I'm still scratching my
>> head trying to understand what finding the BufferedInputStream locked
>> is supposed to signify.
>
> Here's the relevant code block:
>
>                      while (unsafe.tryMonitorEnter(s)) {
>                          unsafe.monitorExit(s);
>                          Thread.sleep(1);
>                      }
>
> This code block waits until it finds that some thread has acquired the
> lock on 's' for at least 1 ms (or else it can miss it and will wait
> further) ...
>
> Since this block is executed for s instanceof BufferedInputStream, what
> this block does, is wait until one of synchronized methods on
> BufferedInputStream is called. The thread that does that, calls read()
> or read(byte[]) and blocks while holding a lock, since the child process
> that is spawned and whose stdout the thread is trying to read in this
> situation does a 10 minute sleep.

Ah I see - thanks for the analysis. So this is a classic 'barrier' 
waiting for the other thread to have reached the synchronized method.

> I think the following could be used instead for the same purpose:
>
>              while (!isLocked(s, 100L)) {
>                  Thread.sleep(1);
>              }
>
> // using the following utility method:
>
>      static boolean isLocked(final Object monitor, long millis) throws
> InterruptedException {
>          Thread t = new Thread() {
>              @Override
>              public void run() {
>                  synchronized (monitor) { }
>              }
>          };
>          t.start();
>          t.join(millis);
>          return t.isAlive();
>      }

Functionally yes this will only return true if the object is locked for 
 >100ms. But it is potentially going to generate a lot of temporary 
threads. And in theory at least the JIT can optimize the run method into 
nothing.

Still it is a cute trick :)

David

>
> Regards, Peter
>
>>
>> David
>> -----
>>
>>> Peter
>>>
>>>>
>>>> Regards, Peter
>>>>
>>>>>
>>>>>
>>>>> On Wed, Feb 26, 2014 at 7:12 AM, Paul Sandoz <paul.sandoz at oracle.com>
>>>>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Out of all the methods on Unsafe i think the
>>>>>> monitorEnter/monitorExit/tryMonitorEnter are the least used and are
>>>>>> very
>>>>>> strong candidates for removal.
>>>>>>
>>>>>> 99% of use-cases are supported by classes in the
>>>>>> java.util.concurrent.locks package.
>>>>>>
>>>>>>
>>>>>> Within the JDK itself it is only used in one JDK test file
>>>>>> test/java/lang/ProcessBuilder/Basic.java:
>>>>>>
>>>>>>                      while (unsafe.tryMonitorEnter(s)) {
>>>>>>                          unsafe.monitorExit(s);
>>>>>>                          Thread.sleep(1);
>>>>>>                      }
>>>>>>
>>>>>> for a test verifying an EOF is received on pending reads and it is
>>>>>> polling
>>>>>> to check when the process builder acquires the lock before
>>>>>> destroying the
>>>>>> process, presumably to avoid some sort of race condition that
>>>>>> occasionally
>>>>>> causes the test to fail.
>>>>>>
>>>>>> I believe this test as been through a number of rounds, i stared at
>>>>>> things
>>>>>> for a bit, but cannot quickly work out a replacement; i lack the
>>>>>> knowledge
>>>>>> on this area.
>>>>>>
>>>>>>
>>>>>> Outside of the JDK i can only find one usage of monitorExit/Enter
>>>>>> (according to grep code) in JBoss modules, and i believe this is
>>>>>> only used
>>>>>> on Java 1.6 to work around some limitations in class loading that
>>>>>> were
>>>>>> fixed in 1.7.
>>>>>>
>>>>>>
>>>>>> Given such very limited use i propose to remove these methods after
>>>>>> having
>>>>>> worked out a fix for ProcessBuilder/Basic.java test.
>>>>>>
>>>>>> Paul.
>>>>>>
>>>>
>>>
>