RFR 8054494: Remove sun.misc.Unsafe.monitorEnter, monitorExit and tryMonitorEnter

[Filipp Zhinkin]

Hi Paul,

On Mon, Jan 12, 2015 at 12:26 PM, Paul Sandoz <paul.sandoz at oracle.com>
wrote:

>
> On Jan 12, 2015, at 1:51 AM, David Holmes <david.holmes at oracle.com> wrote:
>
> > Hi Paul,
> >
> > On 10/01/2015 1:52 AM, Paul Sandoz wrote:
> >> Hi,
> >>
> >>
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8054494-dev-unsafe-monitor-remove/webrev/
> >>
> >> This "forest" webrev [*] removes the monitor related methods from
> Unsafe.
> >
> > Overall looks good to me. For hotspot runtime we also require copyright
> notices be updated.
> >
>
> Thanks, i updated locally.
>
>
> >> The temptation to cleanup code in unsafe.cpp was resisted. I gather now
> that HotSpot Express no longer exists the grouping of registered native
> methods can be cleaned up plus there is dead code related to prefetch
> support. I propose to cleanup in a future patch.
> >
> > Yep lots of historical baggage in there.
> >
> >> The monitor methods resurface on WhiteBox because i noticed some
> associated Unsafe usages within hotspot Java tests. Rather than attempting
> to change such tests i thought it better to move the existing functionality.
> >
> > Unfortunate. :(
> >
>
> Although serendipitously it might be possible to avoid such WB methods,
> see below.
>
>
> >> Now that WhiteBox has moved to the top level repository i could update
> ProcessBuilder/Basic.java to use WhiteBox. I held off doing that for now
> (it seems a little involved to get the relevant classes on the classpath
> for the executing tests with such a dependency).
> >
> > I think using WB would be a lot clearer/cleaner than what you had to
> implement to determine when the lock was held!
> >
>
> Yeah, i might take another swing at that.
>
> When i rebased my repos this morning I had to update due to the
> integration of:
>
>   https://bugs.openjdk.java.net/browse/JDK-8050486
>
> which added a method to WhiteBox, isMonitorInflated, and
> hotspot/test/compiler/testlibrary/rtm/AbortProvoker.java was updated to not
> use Unsafe. So that leaves just
> hotspot/test/compiler/testlibrary/rtm/BusyLock.java using tryMonitorEnter
> and monitorExit:
>
>   66             if (WB.tryMonitorEnter(monitor)) {
>   67                 try {
>   68                     barrier.await();
>   69                     Thread.sleep(timeout);
>   70                 } finally {
>   71                     WB.monitorExit(monitor);
>   72                 }
>   73             } else {
>   74                 throw new RuntimeException("Monitor should be entered
> by " +
>   75                                            "::run() first.");
>   76             }
>
> Could we replace that with:
>
>   if (!WB.isMonitorHeld(monitor)) {
>     synchronized(monitor) {
>         ...
>     }
>   } else {
>     ...
>   }
>
> ?
>
> If so we would no longer require any updates to white box and i could also
> remove ObjectSynchronizer::jni_try_enter method. Plus
> jdk/test/java/lang/ProcessBuilder/Basic.java could be updated to use
> WB.isMonitorHeld method.
>
> Filipp, I noticed you provided the fix for JDK-8050486 so perhaps you may
> know if BusyLock can be updated as suggested.
>
> It could be updated to use synchronized-block instead of Unsafe/WB, see
attached diff.

Regards,
Filipp.


>
> > Also the:
> >
> >  System.out.println(thread.getState());
> >
> > seems to be leftover debugging code?
> >
>
> Removed locally.
>
>
> >> A JPRT job using the hotspot testset passed. Is that the correct
> testset to use?
> >
> > Yes. It also has to be used when doing the forest push.
> >
>
> How does one do that?
>
> Paul.
>
-------------- next part --------------
diff -r dce212d40835 test/compiler/testlibrary/rtm/BusyLock.java
--- a/test/compiler/testlibrary/rtm/BusyLock.java	Tue Dec 30 21:09:33 2014 +0300
+++ b/test/compiler/testlibrary/rtm/BusyLock.java	Mon Jan 12 13:30:01 2015 +0300
@@ -24,9 +24,6 @@
 
 package rtm;
 
-import com.oracle.java.testlibrary.Utils;
-import sun.misc.Unsafe;
-
 import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.CyclicBarrier;
 
@@ -42,7 +39,6 @@
     // Following field have to be static in order to avoid escape analysis.
     @SuppressWarnings("UnsuedDeclaration")
     private static int field = 0;
-    private static final Unsafe UNSAFE = Utils.getUnsafe();
     protected final Object monitor;
     protected final int timeout;
 
@@ -59,18 +55,9 @@
     @Override
     public void run() {
         try {
-            // wait until forceAbort leave monitor
-            barrier.await();
-            if (UNSAFE.tryMonitorEnter(monitor)) {
-                try {
-                    barrier.await();
-                    Thread.sleep(timeout);
-                } finally {
-                    UNSAFE.monitorExit(monitor);
-                }
-            } else {
-                throw new RuntimeException("Monitor should be entered by " +
-                                           "::run() first.");
+            synchronized (monitor) {
+                barrier.await();
+                Thread.sleep(timeout);
             }
         } catch (InterruptedException | BrokenBarrierException e) {
             throw new RuntimeException("Synchronization error happened.", e);
@@ -79,7 +66,6 @@
 
     public void syncAndTest() {
         try {
-            barrier.await();
             // wait until monitor is locked by a ::run method
             barrier.await();
         } catch (InterruptedException | BrokenBarrierException e) {