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

[Thomas Stüfe]

Hi David,

works fine on AIX, as it did before.

Had a look at the change, some very small nits:

- We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
pthread_cond_t). Are the rules not that each header should be
self-contained? If yes, should os_posix.hpp not include the relevant OS
headers for the pthread_... types?

- the coding around dlopen/dlsym would be a bit more readable if you were
to define types for the function pointers, that would save you from
spelling them out each time.

e.g.:

typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
...
static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
dlsym(...);

...Thomas


On Mon, May 29, 2017 at 6:19 AM, David Holmes <david.holmes at oracle.com>
wrote:

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