RFR (S): 8028280 ParkEvent leak when running modified runThese which only loads classes

[David Holmes]

Hi David,

Apologies for the delay in getting back to this, and even more so that I 
won't be able to respond further as I'll be on vacation the next two 
weeks. Fortunately no major issues that would need further discussion :)

src/os/*/vm/os_*.cpp

So ... os::naked_sleep is now morphed into os::naked_short_sleep and is 
always really "naked" ie it uses an OS call. Ok.

Why does the windows version not have:

  assert(ms < 1000, "Long sleep will break Thread.interrupt() semantics");

?

What is your concern with Thread.interrupt here? These sleeps aren't 
involved in Thread.sleep; and the parking logic only looks for 
interrupts at specific points (the existing SpinAcquire code will not 
return if the thread is interrupted). The main issue with long blocking 
is that the thread is not undergoing a state transition and so a 
safepoint will not be reachable until the sleep is over (and we reach a 
point where we check for a safepoint). I'd be worried that a 1 second 
sleep might cause a problem with safepoints. Of course this depends on 
what the caller does before using naked_short_sleep.

---

src/share/vm/runtime/thread.hpp

     // Low-level leaf-lock primitives used to implement synchronization
     // and native monitor-mutex infrastructure.
     // Not for general synchronization use.
+   // Direct naked OS primitives, safe to use standalone in lock impl

Your comment really only applies to SpinAcquire, not the set of methods 
that the original comment block refers to.

---

src/share/vm/runtime/park.cpp

  91   // Note that if we didn't have the TSM/immortal constraint, then
  92   // when reattaching, above, we could trim the list.

What is the "reattaching, above" you are referring to? This comment seem 
more directed to the old code than the new ??

---

Overall this seems okay but I do wonder about the change in performance 
profile. The scenario that caused the leak indicates a high-level of 
contention. Have any of the synchronization benchmarks been run as a 
sanity check?

Thanks,
David

On 26/11/2013 8:17 PM, David Simms wrote:
>
> Updated patch to avoid Solaris RT link requirement...
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8028280
> Webrev: http://cr.openjdk.java.net/~dsimms/8028280/
> <http://cr.openjdk.java.net/%7Edsimms/8028280/>
>
> Rerun testing including vm.quick-pcl.testlist on all platforms.
>
>
> On 11/21/2013 04:36 PM, David Simms wrote:
>>
>> Sorry,
>>
>> Some test failures due to Solaris ld not finding nanosleep...
>>
>>
>> On 11/21/2013 04:03 PM, David Simms wrote:
>>>
>>> Small fix for resource leakage with concurrent free list handling
>>> down in park.cpp
>>>
>>> I did modify Thread::SpinAcquire() to be independent of park event
>>> (naked OS functions only), since this method doesn't need to follow
>>> Thread.interrupt() semantics (park doesn't make sense here).
>>>
>>> This allowed a simple spin lock around the resources without worrying
>>> about the fact that park is used in runtime locks.
>>>
>>> Perhaps SpinAcquire/Release could be spit out to a separate
>>> stand-alone util class, a some point in the future.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028280
>>> Webrev: http://cr.openjdk.java.net/~dsimms/8028280/
>>> <http://cr.openjdk.java.net/%7Edsimms/8028280/>
>>>
>>> Testing: vm testbase, BigApps (30 mins), jtreg (all 64bit platforms)
>>> (...almost completed)
>>>
>>>
>>> /David Simms
>>
>