RFR: 8087322: Implement a Semaphore utility class
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Jun 24 10:59:04 UTC 2015
Hi all,
Updated webrev:
http://cr.openjdk.java.net/~stefank/8087322/webrev.04
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.
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