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

[Paul Sandoz]

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.
 

> 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.