RFR: 8087322: Implement a Semaphore utility class (original) (raw)

Erik Helin erik.helin at oracle.com
Mon Jun 15 12:03:31 UTC 2015

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 osposix 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 semapost/semawait 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 jiosnprintf(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 semawait, I found the following: >> >>2433 while ((ret = ::semawait(&sigsem)) == 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.cpp. I've moved the class declaration >>>>to semaphore.hpp, but the implementation is left in the os.hpp. I >>>>considered creating semaphore.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 >>>>oslinux.cpp, as an example, the code uses both the existing Semaphore >>>>class and the global ::semwait and ::sempost 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 IMPLEMENTSSEMAPHORECLASS 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 osbsd.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 >>>> >>

More information about the hotspot-dev mailing list