RFR: 8087322: Implement a Semaphore utility class
David Holmes
david.holmes at oracle.com
Fri Jun 26 06:48:55 UTC 2015
Hi Dean,
On 26/06/2015 7:35 AM, Dean Long wrote:
> 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");
That amounts to a spurious failure, suggesting the semaphore was not
available when in fact it may have been. The retry is semantically better.
Cheers,
David
>
> 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