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

Dean Long dean.long at oracle.com
Thu Jun 25 21:35:32 UTC 2015

On 6/25/2015 9:42 AM, Stefan Karlsson wrote:

Hi all,

Updated webrev: http://cr.openjdk.java.net/~stefank/8087322/webrev.05.delta http://cr.openjdk.java.net/~stefank/8087322/webrev.05 - Removed unnecessary and incorrect initialization of semaphore - Fixed includes - Loop around semtrywait if EINTR is reported

How about this instead?

1061 bool PosixSemaphore::trywait() { 1062 int ret = sem_trywait(&_semaphore); 1068 assert_with_errno(ret == 0 || (errno == EAGAIN || errno == EINTR), "trywait failed");

dl

- Removed unnecessary NULL check on windows

- Fixed the default value for signal(uint count), on windows. - Moved Sempahore implementation from semaphore.cpp to semaphore.hpp I skipped implementing some of the suggestions that were not essential for this patch or that we haven't reached an agreement on. It doesn't mean that I don't think we should do some of the cleanups. If we could get the structural changes in place (and bug fixes), then we can fix some of the details as follow-up RFEs. Thanks, StefanK On 2015-06-24 23:31, Stefan Karlsson wrote: 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 osxxx.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/osbsd.cpp 1971 OSXSemaphore::OSXSemaphore(uint value) : semaphore(0) { I think that initializer for semaphore isn't right. semaphore is a (OSX) semaphoret. I think that initializer only accidentally compiles at all. This is one yet another case of pre-existing code. ------------------------------------------------------------------------------ src/os/bsd/vm/semaphorebsd.hpp 28 #include "memory/allocation.hpp" 29 30 #include <semaphore.h> 31 32 #ifndef APPLE 33 # include "semaphoreposix.hpp" 34 #else 35 // OS X doesn't support unamed POSIX semaphores, so the implementation in osposix.cpp can't be used. 36 37 class OSXSemaphore : public CHeapObj{ 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/osposix.cpp 1021 #define checkwitherrno(checktype, cond, _msg) _ _1022 do { _ _1023 int err = errno; _ 1024 checktype(cond, errmsg("%s; error='%s' (errno=%d)", msg, _strerror(err), err)); _ 1025 } while (false) 1026 1027 #define assertwitherrno(cond, msg) checkwitherrno(assert, cond, msg) 1028 #define guaranteewitherrno(cond, msg) checkwitherrno(guarantee, cond, msg) We already have assertstatus in debug.hpp. It might be better to add a corresponding guaranteestatus there, and use those here. 1) The comment above vmassertstatus 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 warnfailcommitmemory(char* addr, sizet size, bool exec, 2588 int err) { 2589 warning("INFO: os::commitmemory(" PTRFORMAT ", " SIZEFORMAT 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/osposix.cpp 1053 while ((ret = semwait(&semaphore)) == -1 && errno == EINTR) { I would probably instead use (ret = semwait(&semaphore)) != 0 e.g. while !successful. Sure. ------------------------------------------------------------------------------ src/os/posix/vm/osposix.cpp 1059 bool PosixSemaphore::trywait() { 1060 bool succeeded = semtrywait(&semaphore) == 0; 1061 1062 assertwitherrno(succeeded || errno == EAGAIN, "trywait failed"); 1063 1064 return succeeded; 1065 } semtrywait can also fail with EINTR. Will fix the assert. ------------------------------------------------------------------------------ src/os/posix/vm/osposix.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/oswindows.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/semaphorebsd.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 createtimespec 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/semaphorewindows.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/semaphorewindows.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 semaphorexxx.hpp files in the semaphore.cpp file. But sure, I can inline them in the .hpp file. Thanks, StefanK ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list