RFR: 8087322: Implement a Semaphore utility class
Stefan Karlsson
stefan.karlsson at oracle.com
Thu Jun 25 16:42:49 UTC 2015
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
- 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