(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
David Holmes
david.holmes at oracle.com
Fri Jun 2 00:00:34 UTC 2017
As is typical my change caused a breakage on newer Mac OS (Sierra)
systems that we don't have in JPRT:
https://bugs.openjdk.java.net/browse/JDK-8181451
Ironically the proposed fix is the same typedef cleanup that Thomas had
suggested earlier, but which I didn't take up. <sigh>
I'm trying to find a system I can actually investigate this on. IIUC
these newer Mac's do have clock_gettime (others don't!), but we don't
know how well it works.
David
On 31/05/2017 6:50 AM, David Holmes wrote:
> Hi Dan,
>
> On 31/05/2017 1:11 AM, Daniel D. Daugherty wrote:
>> 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.
>
> Okay - thanks again.
>
>> src/os/posix/vm/os_posix.hpp
>> No comments.
>
> Okay I'm leaving the #includes as-is.
>
>> 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.
>
> Right this is single-threaded VM init.
>
>> An alternative would be to set _use_clock_monotonic_condattr
>> to true only when _pthread_condattr_setclock() returns 0.
>
> Yes - fixed.
>
>> 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.
>
> Fixed.
>
>> 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.
>
> Fixed.
>
>> L1749: to_abstime(&abst, millis * (NANOUNITS/MILLIUNITS),
>> false);
>> nit - please add spaces around the '/' operator.
>
> Fixed.
>
>> 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...
>
> Thanks again.
>
> David
>
>> 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
>>>>>
>>
More information about the hotspot-dev
mailing list