RFR: 8087322: Implement a Semaphore utility class

Dean Long dean.long at oracle.com
Fri Jun 26 08:06:00 UTC 2015


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");

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