RFR: 8087322: Implement a Semaphore utility class

[David Holmes]

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.

I don't see the point of the "max" value in the API given it can't used 
on all platforms.

The posix semaphore functions return -1 and set errno on error, so any 
guarantees (asserts are normally used here) should do the appropriate 
errno string conversion.

Defining operations as Unimplemented on Windows is bad form. I see no 
reason these should be unimplemented as WaitForSingleObject handles the 
required semantics.

I'm not a fan of the creeping use of "unit tests" inside the main 
sources - have I missed some memo on that? Single-threaded unit tests 
for something like Semaphore seems to only be paying lip-service to the 
testing aspect anyway.

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