RFR: 8087322: Implement a Semaphore utility class
Dean Long
dean.long at oracle.com
Fri Jun 26 08:10:33 UTC 2015
On 6/26/2015 1:06 AM, Dean Long wrote:
> On 6/25/2015 11:48 PM, David Holmes wrote:
>> 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.
>>
>
> OK. I see that we already have uses like this:
>
> assert
> <http://ipw83120.uk.oracle.com:8080/source/s?defs=assert&project=jdk9-dev>(!sr_semaphore
> <http://ipw83120.uk.oracle.com:8080/source/xref/jdk9-dev/hotspot/src/os/linux/vm/os_linux.cpp#sr_semaphore>.trywait
> <http://ipw83120.uk.oracle.com:8080/source/xref/jdk9-dev/hotspot/src/os/linux/vm/os_linux.cpp#trywait>(),"invalid
> semaphore state");
>
Oops. Didn't mean to include opengrok markups. Should have been:
assert(!sr_semaphore.trywait(), "invalid semaphore state");
dl
> where we can't tolerate a spurious failure.
>
> dl
>
>> 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