RFR: 8194860: Cleanup Semaphore timed-wait time calculations

[coleen.phillimore at oracle.com]

On 1/17/19 5:19 PM, 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.

One mess down, many to go ...

Thanks for fixing this.
Coleen

>
> 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
>>