RFR: 8087322: Implement a Semaphore utility class

[David Holmes]

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

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

Thanks.

>
> BTW, while looking at the return values of sema_wait, I found the following:
>
> 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.

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

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.

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

Thanks,
David
-----

>> 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?
>
> Thanks,
> StefanK
>>
>> Thanks,
>> David
>>
>> 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 [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
>>>
>