RFR: 8087322: Implement a Semaphore utility class

[Erik Helin]

On 2015-06-15, David Holmes wrote:
> 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.

I would also like to see a better testing framework with tests being in
a separate file and/or folder, but giving that the choice right now is a
unit test or no test at all, I'd rather have the unit test. Anyhow, if
we want better unit testing support, then that belongs in a separate
patch.

I also don't see how you would test this without writing the test as a
unit test? An "external" test won't be able to assert anything about the
VM's own Semaphore class.

Thanks,
Erik

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