RFR: 8087322: Implement a Semaphore utility class

Stefan Karlsson stefan.karlsson at oracle.com
Mon Jun 22 14:28:08 UTC 2015


Hi David,

Updated webrevs:
  http://cr.openjdk.java.net/~stefank/8087322/webrev.03.delta
  http://cr.openjdk.java.net/~stefank/8087322/webrev.03

Comments inlined:

On 2015-06-19 03:27, David Holmes wrote:
> Hi Stefan,
>
> Sorry for the delay ...
>
> On 18/06/2015 3:59 AM, Stefan Karlsson wrote:
>> Hi again,
>>
>> Here's a version that gets rid of the max parameter:
>
> Great! This is looking really good!

Thanks.

>
>> http://cr.openjdk.java.net/~stefank/8087322/webrev.02.delta
>>   http://cr.openjdk.java.net/~stefank/8087322/webrev.02
>>
>> The patch also contains a few minor cleanups and removes the redundant
>> BsdSemaphore::trywait(), which is already defined in PosixSemaphore.
>
> The interaction with the sr_semaphore complicates things. If we push 
> its API up to being the Semaphore API then we have to implement unused 
> functionality on Windows (and perhaps AIX?). If instead we subclass 
> Semaphore to extend the API then we have to duplicate some of the 
> code, even across POSIX systems. :( Unfortunate but I don't see a 
> better approach than what you have chosen (and the Apple vs other-BSD 
> also complicates things).
>
> One thing I am tempted to clean up a little more is the timedwait 
> call. PosixSemaphore timedwait takes a timespec to pass to the 
> underlying sem_timedwait. But the primary API is timed_wait(long secs, 
> int nsecs) which each PosixSemaphore subclass has to add, and each 
> then has to perform the "unpackTime" mechanics (which in itself is 
> something screaming to be cleaned up). What if PosixSemaphore defined:
>
> class PosixSemaphore : public Semaphore {
>   public:
>   PosixSemaphore(uint value = 0) : Semaphore(value) {}
>
>   bool trywait();
>   bool timedwait(unsigned int sec, int nsec);
>
>   protected:
>   virtual void rel_to_abs_timespec(struct timespec* ts, unsigned int 
> sec, int nsec) = 0;
> };
>
> then:
>
> bool os::PosixSemaphore::timedwait(unsigned int sec, int nsec) {
>   struct timespec ts;
>   rel_to_abs_timespec(&ts, sec nsecs);
>   while (true) {
>     int result = sem_timedwait(&_semaphore, &ts);
>     ...
>   }
> }
>
> and then just define the rel_to_abs_timespec functions for BSD, linux 
> and Solaris subclasses. ?

I did something similar while prototyping alternative implementations of 
this. I named the function to PosixSemaphore::create_timespec, but I can 
change that name if you want to.

> ---
>
> The sr_semaphore appears to be being initialized by C++ static 
> initialization, which is a concern for me, primarily because Semaphore 
> construction requires allocation support (it's a CHeapObj) and 
> utilizes NMT. I get very nervous when NMT gets involved prior to VM 
> initialization. Can we make sr_semaphore a pointer instead and 
> initialize it in one of the os::init methods?

The current code doesn't use the CHeapObj new operator, so NMT isn't 
involved here. I'd prefer if this could be changed as a separate RFE.

>
> ---
>
> A few specific comments ...
>
> src/os/posix/vm/os_posix.cpp
>
> Two nits that I know have been copied from the existing code:
>
> 1061   while (1) {
>
> I prefer while (true)
>
> 1069     } else {
> 1070       return false;
> 1071     }
>
> This is returning false when an error has occurred. At a minimum there 
> should be an assert(false, "...") so that we detect this in debug builds.

Done.

>
> ---
>
> src/share/vm/utilities/semaphore.hpp
>
> Can we not implement this:
>
>  53   void signal();
>  54   void signal(uint count);
>
> as simply:
>
>   void signal(uint count = 1);
>
> ?

Done.

> ---
>
> src/share/vm/utilities/semaphore.cpp:
>
> The logic in test_semaphore_many isn't quite right. For example if you 
> call it with value=0, max=2, increment=1, then you will end up 
> performing two signals() but only one wait().

Fixed.

Thanks,
StefanK

>
> Thanks,
> David
> -----
>
>
>> Thanks,
>> StefanK
>>
>> On 2015-06-15 22:28, Stefan Karlsson wrote:
>>> Hi again,
>>>
>>> I've hopefully addressed some of Kim's and David's concerns with the
>>> following version:
>>>
>>> http://cr.openjdk.java.net/~stefank/8087322/webrev.01.delta/
>>> http://cr.openjdk.java.net/~stefank/8087322/webrev.01/
>>>
>>> Changes in the current version of the patch:
>>>
>>> - Created a POSIX version that is used by Linux and Solaris (and maybe
>>> AIX?).
>>>
>>> - Use asserts instead of guarantees. (I've got offline feedback that
>>> others preferred at least some of the guarantees.)
>>>
>>> - Print the errno value and the errno string in the posix version
>>>
>>> - Removed trywait and timedwait from the Semaphore class and added
>>> that functionality in platform-specific semaphore classes. This gets
>>> rid of the Unimplemented() functions in os_windows.cpp.
>>>
>>> - I removed the IMPLEMENTS_SEMAPHORE_CLASS define.
>>>
>>> Some comments about the current patch:
>>>
>>> - I have not removed the 'max' parameter, since I haven't yet figured
>>> out what the max value should be used for windows.
>>>
>>> - OS X doesn't implement unamed POSIX semaphores, so I have to go
>>> through hoops to compile out the POXIS version when building on OS X.
>>>
>>> - I had to add -lrt to get the patch to link on Solaris.
>>>
>>> Tested with JPRT,
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2015-06-12 17:21, Stefan Karlsson wrote:
>>>> Hi all,
>>>>
>>>> Please review this patch to create a Semaphore utility class. I need
>>>> this class to implementing faster GC thread synchronization [1].
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8087322/webrev.00/
>>>> https://bugs.openjdk.java.net/browse/JDK-8087322
>>>>
>>>> Some of our platforms already implement a Semaphore class, but those
>>>> classes are hidden inside os_<os>.cpp. I've moved the class
>>>> declaration to semaphore.hpp, but the implementation is left in the
>>>> os_<os>.hpp. I considered creating semaphore_<os>.cpp files, but I
>>>> ended up having to restructure too much code and I wanted to focus on
>>>> the feature in [1] instead. Should I create a RFE to move the
>>>> semaphore implementations?
>>>>
>>>> There seems to be another opportunity to cleanup the code. If we take
>>>> os_linux.cpp, as an example, the code uses both the existing
>>>> Semaphore class and the global ::sem_wait and ::sem_post functions.
>>>> We might want to consider unifying that code.
>>>>
>>>> Since HotSpot is built on platforms that I don't have access to and
>>>> can't test, I've added the IMPLEMENTS_SEMAPHORE_CLASS define. So,
>>>> that I can detect if the the current platform implements the
>>>> Semaphore class, and choose what synchronization primitive to use by
>>>> the GC.
>>>>
>>>> The os_bsd.cpp file has support for "non-apple BSD", but I've only
>>>> tested this patch with OS X.
>>>>
>>>> This patch has been tested in JPRT and our nightly testing together
>>>> with the patches in [1]. The patch also contains a few unit tests in
>>>> semaphore.cpp.
>>>>
>>>> Thanks,
>>>> StefanK
>>>>
>>>> [1]
>>>> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2015-June/013829.html 
>>>>
>>>>
>>>
>>



More information about the hotspot-dev mailing list