(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems

[David Holmes]

Dan, Robbin, Thomas,

Okay here is the final ready to push version:

http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/

this fixes all Dan's nits and refactors the time calculation code as 
suggested by Robbin.

Thomas: if you are around and able, it would be good to get a final 
sanity check on AIX. Thanks.

Testing:
  - JPRT: -testset hotspot
          -testset core

- manual:
    - jtreg:java/util/concurrent
    - various little test programs that try to validate sleep/wait times 
to show early returns or unexpected delays

Thanks again for the reviews.

David

On 29/05/2017 10:29 AM, David Holmes wrote:
> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>> On 5/26/17 1:27 AM, David Holmes wrote:
>>> Robbin, Dan,
>>>
>>> Below is a modified version of the refactored to_abstime code that 
>>> Robbin suggested.
>>>
>>> Robbin: there were a couple of issues with your version. For relative 
>>> time the timeout is always in nanoseconds - the "unit" only tells you 
>>> what form the "now_part_sec" is - nanos or micros. And the 
>>> calc_abs_time always has a deadline in millis. So I simplified and 
>>> did a little renaming, and tracked max_secs in debug_only instead of 
>>> returning it.
>>>
>>> Please let me know what you think.
>>
>> Looks OK to me. Nit comments below...
> 
> Thanks Dan - more below.
> 
>>>
>>>
>>> // Calculate a new absolute time that is "timeout" nanoseconds from 
>>> "now".
>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or 
>>> micros depending
>>> // on which clock is being used).
>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong 
>>> now_sec,
>>>                           jlong now_part_sec, jlong unit) {
>>>   time_t max_secs = now_sec + MAX_SECS;
>>>
>>>   jlong seconds = timeout / NANOUNITS;
>>>   timeout %= NANOUNITS; // remaining nanos
>>>
>>>   if (seconds >= MAX_SECS) {
>>>     // More seconds than we can add, so pin to max_secs.
>>>     abstime->tv_sec = max_secs;
>>>     abstime->tv_nsec = 0;
>>>   } else {
>>>     abstime->tv_sec = now_sec  + seconds;
>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>     if (nanos >= NANOUNITS) { // overflow
>>>       abstime->tv_sec += 1;
>>>       nanos -= NANOUNITS;
>>>     }
>>>     abstime->tv_nsec = nanos;
>>>   }
>>> }
>>>
>>> // Unpack the given deadline in milliseconds since the epoch, into 
>>> the given timespec.
>>> // The current time in seconds is also passed in to enforce an upper 
>>> bound as discussed above.
>>> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong 
>>> now_sec) {
>>>   time_t max_secs = now_sec + MAX_SECS;
>>>
>>>   jlong seconds = deadline / MILLIUNITS;
>>>   jlong millis = deadline % MILLIUNITS;
>>>
>>>   if (seconds >= max_secs) {
>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>     abstime->tv_sec = max_secs;
>>>     abstime->tv_nsec = 0;
>>>   } else {
>>>     abstime->tv_sec = seconds;
>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>   }
>>> }
>>>
>>>
>>> static void to_abstime(timespec* abstime, jlong timeout, bool 
>>> isAbsolute) {
>>
>> There's an extra blank line here.
> 
> Fixed.
> 
>>>
>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>
>>>   if (timeout < 0) {
>>>     timeout = 0;
>>>   }
>>>
>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>
>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>     struct timespec now;
>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>     assert_status(status == 0, status, "clock_gettime");
>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>   } else {
>>>
>>> #else
>>>
>>>   { // Match the block scope.
>>>
>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>
>>>     // Time-of-day clock is all we can reliably use.
>>>     struct timeval now;
>>>     int status = gettimeofday(&now, NULL);
>>>     assert(status == 0, "gettimeofday");
>>
>> assert_status() is used above, but assert() is used here. Why?
> 
> Historical. assert_status was introduced for the pthread* and other 
> posix funcs that return the error value rather than returning -1 and 
> setting errno. gettimeofday is not one of those so still has the old 
> assert. However, as someone pointed out a while ago you can use 
> assert_status with these and pass errno as the "status". So I did that.
> 
>>
>>>     if (isAbsolute) {
>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>     }
>>>     else {
>>
>> Inconsistent "else-branch" formatting.
>> I believe HotSpot style is "} else {"
> 
> Fixed.
> 
>>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>     }
>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>   }
>>>
>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >= 
>>> nanos_per_sec");
>>
>> Why does the assert mesg have "nanos_per_sec" instead of 
>> "NANOSECS_PER_SEC"?
> 
> No reason. Actually that should now refer to NANOUNITS. Hmmm I can not 
> recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly an 
> oversight.
> 
>> There's an extra blank line here.
> 
> Fixed.
> 
> Will send out complete updated webrev soon.
> 
> Thanks,
> David
> 
>>>
>>> }
>>
>> Definitely looks and reads much cleaner.
>>
>> Dan
>>