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

David Holmes david.holmes at oracle.com
Mon Jun 15 11:36:14 UTC 2015

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.

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