RFR: 8087322: Implement a Semaphore utility class

[David Holmes]

On 15/06/2015 6:41 PM, Erik Helin wrote:
> On 2015-06-15, 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?
>>
>>>
>>> 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.
>
> Well, if the tests are easy to implement and verify, why not implement
> them? There is of course a need for more testing, especially those parts
> that are not covered by the unit tests, but at least those test won't
> have to care about verifying what is covered by these unit tests.

I don't like seeing tests split up either. Full blown functional unit 
tests don't belong in the primary source code in my opinion. So I'd 
rather see a more complete separate test, than a simple "sanity 
checking" internal unit test that will still need to be complemented by 
further external testing.

Cheers,
David

>> It tests boundary conditions. What do other Reviewers think. Should I remove
>> the tests?
>
> No, keep them. Even though you can only test the single-threaded case,
> it helps me understand and review the change. I also appreciate that you
> took the time to write the tests!
>
> Thanks,
> Erik
>
>> 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
>>>>
>>