RFR: 8087322: Implement a Semaphore utility class

Wed Jun 24 21:31:55 UTC 2015

Hi Kim,

On 2015-06-24 22:28, Kim Barrett wrote:
> 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.

This is one yet another case of pre-existing code.

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

OK.

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

I'll change it.

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

1) The comment above vmassert_status says:

// This version of vmassert is for use with checking return status from
// library calls that return actual error values eg. EINVAL,
// ENOMEM etc, rather than returning -1 and setting errno.
// When the status is not what is expected it is very useful to know
// what status was actually returned, so we pass the status variable as
// an extra arg and use strerror to convert it to a meaningful string
// like "Invalid argument", "out of memory" etc

but called library calls actually do return -1 and sets errno. Maybe the 
comment is too specific?

2) I modeled the error message with this code in mind:

2587 static void warn_fail_commit_memory(char* addr, size_t size, bool exec,
2588                                     int err) {
2589   warning("INFO: os::commit_memory(" PTR_FORMAT ", " SIZE_FORMAT
2590           ", %d) failed; error='%s' (errno=%d)", addr, size, exec,
2591           strerror(err), err);
2592 }

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

Yes, but they are not specific to non-__APPLE__ code, so I chooe to put 
it outside that 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.

Sure.

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

Will fix the assert.

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

This pre-existing code can be fixed as a separate RFE.

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

I'll remove it.

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

Yes. I put it in the class so that I wouldn't have to create a prefix 
for currenttime and to make it obvious that only the OSXSemaphore uses 
that function.

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

I'll fix.

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

This is a bug that I thought I had fixed. I'll change it.

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

Because I don't think they are performance critical. Another reason is 
that one of my prototypes forward declared SemaphoreImpl in 
semaphore.hpp and only included the platform specific semaphore_xxx.hpp 
files in the semaphore.cpp file.

But sure, I can inline them in the .hpp file.

Thanks,
StefanK
>
> ------------------------------------------------------------------------------
>