RFR: 8194860: Cleanup Semaphore timed-wait time calculations

[David Holmes]

Hi Kim,

Responses inline.

On 21/01/2019 6:42 pm, Kim Barrett wrote:
>> On Jan 20, 2019, at 9:29 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Here's the updated webrev:
>>
>> http://cr.openjdk.java.net/~dholmes/8194860/webrev.v2
>>
>> Only changes are in os_posix.cpp:
>>
>> - As Coleen requested I statically initialized the various static variables to simplify the conditional compilation for Solaris and removed the dynamic initialization to NULL/false.
>>
>> - I had a bug (exposed on old OS X versions) where I was using clock_gettime even though it might not be available.
> 
> Looks good.

Thanks for looking at it.

> A few questions / comments about mostly pre-existing code.
> 
> ------------------------------------------------------------------------------
> src/hotspot/os/posix/os_posix.cpp
> 1630 static pthread_condattr_t _condAttr[1];
> 1634 static pthread_mutexattr_t _mutexAttr[1];
> 
> Pre-existing.  Why are these one element arrays, rather than just
> single objects?

I don't recall the rationale but its a pattern that's been in the code 
for years.

> ------------------------------------------------------------------------------
> src/hotspot/os/posix/os_posix.cpp
> 1735   int (*condattr_setclock_func)(pthread_condattr_t*, clockid_t) =
> 1736     (int (*)(pthread_condattr_t*, clockid_t))dlsym(RTLD_DEFAULT,
> 1737                                                    "pthread_condattr_setclock");
> 1738   if (condattr_setclock_func != NULL) {
> 1739     _pthread_condattr_setclock = condattr_setclock_func;
> 1740   }
> 
> Pre-existing.  Why the local condattr_setclock_func?  Why not just
> directly set _pthread_condattr_setclock from the dlsym lookup?  This
> isn't like the earlier case with _clock_gettime &etc, where there are
> additional checks to be performed before accepting the library
> function.

I just copied the existing patterns when I added this.

> ------------------------------------------------------------------------------
> src/hotspot/os/posix/os_posix.hpp
>   136   static void to_RTC_abstime(timespec* abstime, int64_t millis);
> 
> I thought about suggesting returning a timespec rather than taking a
> pointer to one as an argument. That would be more usual C++ style, and
> would avoid any possibility of accidentally passing in a NULL pointer.
> But it would require doing some research on the state of copy elision
> in some compilers that I'm less familiar with. Doing so would also
> suggest doing the same for to_abstime.

I'm still a C programmer at heart and find myself much more comfortable 
using pointers this way - as they get used by the C libraries. I don't 
know when C++ will or won't make additional copies when returning 
structs. I always get concerned by C++ code that looks to me like it is 
returning a stack-allocated local instance.

Thanks,
David
-----

> ------------------------------------------------------------------------------
>