RFR: 8087322: Implement a Semaphore utility class
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Jun 24 12:17:43 UTC 2015
On 2015-06-24 14:10, David Holmes wrote:
> 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.
Thanks for reviewing!
StefanK
>
>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>>
More information about the hotspot-dev
mailing list