RFR: 8087322: Implement a Semaphore utility class
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 sem_trywait 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 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
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>
>
More information about the hotspot-dev
mailing list