(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
David Holmes
david.holmes at oracle.com
Tue May 30 20:50:51 UTC 2017
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