RFR: 8087322: Implement a Semaphore utility class

[David Holmes]

Hi Stefan,

On 24/06/2015 8:59 PM, Stefan Karlsson wrote:
> Hi all,
>
> Updated webrev:
>   http://cr.openjdk.java.net/~stefank/8087322/webrev.04

This looks good to me.

> This patch:
>
> - Gets rid of the public inheritance between Semaphore class and the
> [OS]Semaphore classes.
>
> - Semaphore is now a wrapper around the [OS]Semaphore implementation,
> and only expose a limited API (signal, wait). The OS-specific extensions
> to the semaphore API is only exposed and implemented in the
> [OS]Semaphore classes.
>
> - Changed back to use a guarantee in the constructor of the
> [OS]Semaphore classes.
>
> - Added more asserts.
>
> There are a few pre-existing bugs in the OSX implementation, but it
> would be good to handle those as separate RFEs.

Agreed.

Thanks for all your work and perseverance on this.

David

> Thanks,
> StefanK
>
> On 2015-06-22 16:28, Stefan Karlsson wrote:
>> 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>
>