RFR: 8087322: Implement a Semaphore utility class

[Stefan Karlsson]

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?

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

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


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

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

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