RFR: 8087322: Implement a Semaphore utility class
stefan.karlsson at oracle.com
Mon Jun 15 12:52:42 UTC 2015
On 2015-06-15 09:17, David Holmes wrote:
> Hi Stefan,
> On 15/06/2015 4:40 PM, Stefan Karlsson wrote:
>> Hi David,
>> On 2015-06-15 01:47, David Holmes wrote:
>>> Hi Stefan,
>>> Quick response - sorry will try to get back to this later.
>>> I second the comment to define an os_posix implementation and to do it
>>> now rather than defer it for future work. Solaris supports POSIX
>>> semaphore API (as well as the SUS/UI semaphore API). This may boil
>>> down to only two implementations: posix and win32.
>> This adds risk to this patch. Are we sure that the Solaris OS code
>> doesn't rely on some implementation detail of sema_post/sema_wait that
>> is different from how the POSIX semaphores work? For example, how it
>> interacts with signal handlers, etc?
> Solaris supports two different semaphore API's. The POSIX API has
> sem_wait, sem_post etc and operates on sem_t. The other (SUS or UI or
> Solaris-only ?) API has sema_* and operates on sema_t - it is a very
> subtle difference: sem vs sema. Under the covers they are the same
> thing, but the API's themselves have some differences. You can use the
> POSIX API on Solaris, just as on Linux and hopefully BSD (and maybe AIX?)
I'm going to send out a new patch using the posix API for the Semaphore
>>> I don't see the point of the "max" value in the API given it can't
>>> used on all platforms.
>> I don't want it either, but it's needed by the windows implementation. I
>> could get rid of it if we have a sensible max value. Any suggestion on
>> what value to use?
> On Windows pass in whatever MAX windows defines.
Do you know where to find that MAX value?
The document doesn't mention the maximum value. Only the type of it:
_In_ LONG lMaximumCount,
The maximum count for the semaphore object. This value must be
greater than zero.
Should I assume that LONG_MAX is the right value to use?
>>> The posix semaphore functions return -1 and set errno on error, so any
>>> guarantees (asserts are normally used here)
>> I can change to asserts.
>>> should do the appropriate errno string conversion.
>> I followed the surrounding code. For example:
>> 3411 jio_snprintf(msg, sizeof(msg), "Failed to reserve shared
>> memory (errno = %d).", errno);
>> but I see that we print a string at other places. I can change it.
>> BTW, while looking at the return values of sema_wait, I found the
>> 2433 while ((ret = ::sema_wait(&sig_sem)) == EINTR)
>> 2434 ;
>> I wonder if this code is broken. Since it checks the return value
>> against EINTR and not errno.
> sema_wait returns zero or an error code. POSIX sem_wait returns 0/-1
> and sets errno.
OK. I haven't read the Solaris source code, so what you're saying might
>>> Defining operations as Unimplemented on Windows is bad form. I see no
>>> reason these should be unimplemented as WaitForSingleObject handles
>>> the required semantics.
>> But that means adding dead/untested code. That's another kind of bad
> Can you elaborate on why only part of the API is being used on
> Windows? I would have hoped the client code was cross-platform.
The client code that I'm adding doesn't use the timedwait() and
trywait() functions, but the os_<os>.cpp code did.
I'll address this in the coming patch where I minimize the API for
Semaphore and instead have more complexity in the os_<os> files.
>>> I'm not a fan of the creeping use of "unit tests" inside the main
>>> sources - have I missed some memo on that?
>> We have been doing that for many years now.
> "we" have? I've only noticed a few occurrences :)
I remember this discussion from 2011:
I don't feel we need to use this thread to discuss the suitability of
having the unit tests in the .cpp files.
>>> Single-threaded unit tests for something like Semaphore seems to only
>>> be paying lip-service to the testing aspect anyway.
>> It tests boundary conditions. What do other Reviewers think. Should I
>> remove the tests?
>>> On 13/06/2015 1:21 AM, 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 .
>>>> Some of our platforms already implement a Semaphore class, but those
>>>> classes are hidden inside os_<os>.cpp. I've moved the class
>>>> 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 
>>>> 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
>>>> 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
>>>> the patches in . The patch also contains a few unit tests in
More information about the hotspot-dev