RFR: 8087322: Implement a Semaphore utility class

[Stefan Karlsson]

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

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


and:

/lMaximumCount/ [in]

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

OK. I haven't read the Solaris source code, so what you're saying might 
be correct.

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

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:
http://mail.openjdk.java.net/pipermail/hotspot-dev/2011-October/004640.html

:)

I don't feel we need to use this thread to discuss the suitability of 
having the unit tests in the .cpp files.

Thanks,
StefanK

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