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

[Daniel D. Daugherty]

On 5/28/17 10:19 PM, David Holmes wrote:
> Dan, Robbin, Thomas,
>
> Okay here is the final ready to push version:
>
> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/

General
   - Approaching the review differently than last round. This time I'm
     focused on the os_posix.[ch]pp changes as if this were all new code.
   - i.e., I'm going to assume that code deleted from the platform
     specific files is all appropriately represented in os_posix.[ch]pp.

src/os/posix/vm/os_posix.hpp
     No comments.

src/os/posix/vm/os_posix.cpp
     L1518:     _use_clock_monotonic_condattr = true;
     L1522:         _use_clock_monotonic_condattr = false;
         _use_clock_monotonic_condattr could briefly be observed as 'true'
         before being reset to 'false' due to the EINVAL. I think we are
         single threaded at this point so there should be no other thread
         running to be confused by this.

         An alternative would be to set _use_clock_monotonic_condattr
         to true only when _pthread_condattr_setclock() returns 0.

     L1581: // number of seconds, in abstime, is less than current_time 
+ 100,000,000.
     L1582: // As it will be over 20 years before "now + 100000000" will 
overflow we can
     L1584: // of "now + 100,000,000". This places a limit on the 
timeout of about 3.17
         nit - consistency of using ',' or not in 100000000. Personally,
             I would prefer no commas so the comments match MAX_SECS.

     L1703:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
     L1743:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
         nit - please add spaces around the '-' operator.

     L1749:     to_abstime(&abst, millis * (NANOUNITS/MILLIUNITS), false);
         nit - please add spaces around the '/' operator.

src/os/aix/vm/os_aix.hpp
     No comments.

src/os/aix/vm/os_aix.cpp
     No comments.

src/os/bsd/vm/os_bsd.hpp
     No comments.

src/os/bsd/vm/os_bsd.cpp
     No comments.

src/os/linux/vm/os_linux.hpp
     No comments.

src/os/linux/vm/os_linux.cpp
     No comments.

src/os/solaris/vm/os_solaris.hpp
     No comments.

src/os/solaris/vm/os_solaris.cpp
     No comments.


Thumbs up. Don't need to see another webrev if you choose to fix
the bits...

Dan


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