RFR: 8087322: Implement a Semaphore utility class

[Kim Barrett]

On Jun 24, 2015, at 6:59 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
> Hi all,
> 
> Updated webrev:
> http://cr.openjdk.java.net/~stefank/8087322/webrev.04

Structurally fine.  (Stefan would be rightly annoyed with me if I said
otherwise after all the offline discussions we've had.)  There's some
nastiness that results from pre-existing problems in the os_xxx.cpp
files, but cleaning that up is another collection of tasks.

A few easy bugs, some stylistic comments that can be accepted or
declined, but nothing requiring large amounts of rework.

------------------------------------------------------------------------------ 
src/os/bsd/vm/os_bsd.cpp
1971 OSXSemaphore::OSXSemaphore(uint value) : _semaphore(0) {

I think that initializer for _semaphore isn't right.  _semaphore is a
(OSX) semaphore_t.  I think that initializer only accidentally
compiles at all.

------------------------------------------------------------------------------
src/os/bsd/vm/semaphore_bsd.hpp
  28 #include "memory/allocation.hpp"
  29 
  30 #include <semaphore.h>
  31 
  32 #ifndef __APPLE__
  33 # include "semaphore_posix.hpp"
  34 #else
  35 // OS X doesn't support unamed POSIX semaphores, so the implementation in os_posix.cpp can't be used.
  36 
  37 class OSXSemaphore : public CHeapObj<mtInternal>{

This only needs "memory/allocation.hpp" in the Apple case.

This doesn't need <semaphore.h> in the non-Apple case.

In the Apple case, I think the correct include is <mach/semaphore.h>
rather than <semaphore.h>.  I'm not sure why <semaphore.h> is working
at all.

------------------------------------------------------------------------------ 
src/os/posix/vm/os_posix.cpp   
1021 #define check_with_errno(check_type, cond, msg)                                      \
1022   do {                                                                               \
1023     int err = errno;                                                                 \
1024     check_type(cond, err_msg("%s; error='%s' (errno=%d)", msg, strerror(err), err)); \
1025 } while (false)
1026 
1027 #define assert_with_errno(cond, msg)    check_with_errno(assert, cond, msg)
1028 #define guarantee_with_errno(cond, msg) check_with_errno(guarantee, cond, msg)

We already have assert_status in debug.hpp.  It might be better to add
a corresponding guarantee_status there, and use those here.

Also, these macros are only used inside the #ifndef __APPLE__ block.

And welcome to the dark side.  (Higher order macros!)

------------------------------------------------------------------------------ 
src/os/posix/vm/os_posix.cpp   
1053   while ((ret = sem_wait(&_semaphore)) == -1 && errno == EINTR) {

I would probably instead use

  (ret = sem_wait(&_semaphore)) != 0

e.g. while !successful.

------------------------------------------------------------------------------ 
src/os/posix/vm/os_posix.cpp   
1059 bool PosixSemaphore::trywait() {
1060   bool succeeded = sem_trywait(&_semaphore) == 0;
1061 
1062   assert_with_errno(succeeded || errno == EAGAIN, "trywait failed");
1063 
1064   return succeeded;
1065 }

sem_trywait can also fail with EINTR.

------------------------------------------------------------------------------ 
src/os/posix/vm/os_posix.cpp  
1072     } else if (errno == EINTR) {
1073       continue;
1074     } else if (errno == ETIMEDOUT) {

I think ETIMEDOUT should be tested before EINTR.  ETIMEDOUT is the
more interesting and performance relevant case.

------------------------------------------------------------------------------ 
src/os/windows/vm/os_windows.cpp
1905 WindowsSemaphore::~WindowsSemaphore() {
1906   if (_semaphore != NULL) {
1907     ::CloseHandle(_semaphore);
1908   }

I don't think the NULL check is needed here.

------------------------------------------------------------------------------ 
src/os/bsd/vm/semaphore_bsd.hpp
  56   static jlong currenttime();

This function is an implementation detail of the timedwait function,
and could be a file-scoped static near its caller, rather than having
external linkage.

[The PosixSemaphore class is different in this respect, because there
we need to choose between platform-specific definitions of
create_timespec that will be in a different file from the reference,
so external linkage is required.  That situation doesn't apply for
OSXSemaphore.]

------------------------------------------------------------------------------
src/os/windows/vm/semaphore_windows.hpp 
  30 #include <WinBase.h>

I think <windef.h> is sufficient here, and is purportedly smaller.
The .cpp file likely needs more, but is already including <windows.h>.
Also, it looks like we prefer the lowercase form on Windows, even
though the file system is case-insensitive.

------------------------------------------------------------------------------ 
src/os/windows/vm/semaphore_windows.hpp 
  43   void signal(uint count = 0);

Default count of 0 is inconsistent with corresponding classes for
other platforms.

------------------------------------------------------------------------------ 
src/share/vm/runtime/semaphore.cpp
  30 Semaphore::Semaphore(uint value) : _impl(value) {}
  31 Semaphore::~Semaphore()            {}
  32 void Semaphore::signal(uint count) { _impl.signal(count); }
  33 void Semaphore::wait()             { _impl.wait(); }

I'm not sure why these forwarding definitions are out of line in the
.cpp file, rather than inline in the header.  After all, we've now
gone to some trouble to have the wrapped platform-specific
implementation class provide at least that set of operations.

------------------------------------------------------------------------------