RFR: 8087322: Implement a Semaphore utility class

[David Holmes]

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!

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

---

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?

---

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.

---

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);

?
---

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

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