RFR: 8194860: Cleanup Semaphore timed-wait time calculations

[David Holmes]

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.

Thanks,
David

On 18/01/2019 8:19 am, David Holmes wrote:
> Hi Coleen,
> 
> Thanks for looking at this - though I still have to resolve my OS X issue.
> 
> On 17/01/2019 11:04 pm, coleen.phillimore at oracle.com wrote:
>> http://cr.openjdk.java.net/~dholmes/8194860/webrev/src/hotspot/os/posix/os_posix.cpp.frames.html 
>>
>>
>> For os_init2, I don't see why you have to sprinkle #ifndef SOLARIS. 
>> It's just logging, won't the logging be useful in the negative for 
>> solaris? 
> 
> The problem is that regardless of whether the initialization works or 
> fails on Solaris, those "attributes" won't get used on Solaris anyway, 
> so the logging would be misleading/confusing. So I just elided it.
> 
>> Can these three statics default to false/NULL above?  Then you can 
>> save having to have this conditional as well:
>>
>> +#else
>> +  _use_clock_monotonic_condattr = false;
>> +#endif // !SOLARIS
> 
> Yes I should be able to do that (famous last words!).
> 
>> Thank you for resolving the differences between to_abstime and solaris 
>> unpackTime, which was one of the causes of this mess left by the 
>> refactoring.
> 
> There's still a lot of mess in the Solaris code.
> 
> Thanks again,
> David
> 
>> Looks good, with this one suggestion to remove some #ifndef solaris 
>> above.
>> Coleen
>>
>>
>>
>> On 1/16/19 5:06 PM, David Holmes wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8194860
>>> webrev: http://cr.openjdk.java.net/~dholmes/8194860/webrev/
>>>
>>> Details in the bug report, but there was duplication in functionality 
>>> with regards to timeout conversions/calculations, and the os_bsd.cpp 
>>> code was broken due to OSXSemaphore and PosixSemaphore having 
>>> different API's.
>>>
>>> Summary:
>>> - expanded the API to take a relative timeout in millis as well as 
>>> the existing absolute timeout as a timespec:
>>>
>>>   // wait until the given absolute time is reached
>>>   bool timedwait(struct timespec ts);
>>>   // wait until the given relative time elapses
>>>   bool timedwait(int64_t millis);
>>>
>>> The millis version simply converts to a timespec and calls the other 
>>> version e.g
>>>
>>> bool PosixSemaphore::timedwait(int64_t millis) {
>>>   struct timespec ts;
>>>   os::Posix::to_RTC_abstime(&ts, millis);
>>>   return timedwait(ts);
>>> }
>>>
>>> - added os::Posix::to_RTC_abstime to do the relative to absolute 
>>> conversion using the existing routines in os::Posix used for 
>>> PlatformEvent etc.
>>>
>>> - changed callsites to use new millis version
>>>
>>> - I had to rearrange the ifndef SOLARIS in os_posix.cpp to expose 
>>> previously hidden code to Solaris. It isn't all needed by Solaris but 
>>> I chose not to clutter things with too many ifdefs and only excluded 
>>> things that really shouldn't be done on Solaris. Also needed to call 
>>> os::Posix::init on Solaris.
>>>
>>> Testing (still in progress):
>>> - mach 5 tiers 1 - 3 (Linux, Solaris, OS X, Windows)
>>> - test/jdk/jdk/jfr (as suspend/resume is the user of the semaphore code)
>>>
>>> Thanks,
>>> David
>>