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

Daniel D. Daugherty daniel.daugherty at oracle.com
Thu May 25 16:48:54 UTC 2017

On 5/18/17 12:25 AM, David Holmes wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs: Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/

General comment(s):

common/autoconf/flags.m4 No comments.

common/autoconf/generated-configure.sh No comments.

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

src/os/aix/vm/os_aix.cpp No comments; did not try to compare deleted code with os_posix.cpp.

src/os/aix/vm/os_aix.hpp No comments; did not try to compare deleted code with os_posix.hpp.

src/os/bsd/vm/os_bsd.cpp No comments; compared deleted code with os_posix.cpp version; nothing jumped out as wrong.

src/os/bsd/vm/os_bsd.hpp No comments; compared deleted code with os_posix.hpp version; nothing jumped out as wrong.

src/os/linux/vm/os_linux.cpp No comments; compared deleted code with os_posix.cpp version; nothing jumped out as wrong.

src/os/linux/vm/os_linux.hpp No comments; compared deleted code with os_posix.hpp version; nothing jumped out as wrong.

src/os/posix/vm/os_posix.cpp L1401: // Not currently usable by Solaris L1408: // time-of-day clock nit - needs period at end of the sentence

 L1433: // build time support then there can not be
     typo - "can not" -> "cannot"

 L1435: // int or int64_t.
     typo - needs a ')' before the period.

 L1446: // determine what POSIX API's are present and do appropriate
 L1447: // configuration
     nits - 'determine' -> 'Determine'
          - needs period at end of the sentence

 L1455:   // 1. Check for CLOCK_MONOTONIC support
     nit - needs period at end of the sentence

 L1462:   // we do dlopen's in this particular order due to bug in linux
 L1463:   // dynamical loader (see 6348968) leading to crash on exit
     nits - 'we' -> 'We'
          - needs period at end of the sentence

     typo - 'dynamical' -> 'dynamic'

 L1481:     // we assume that if both clock_gettime and clock_getres

support L1482: // CLOCK_MONOTONIC then the OS provides true high-res monotonic clock nits - 'we' -> 'We' - needs period at end of the sentence

 L1486:         clock_gettime_func(CLOCK_MONOTONIC, &tp) == 0) {
     nit - extra space before '=='

 L1487:       // yes, monotonic clock is supported
     nits - 'yes' -> 'Yes'
          - needs period at end of the sentence

 L1491:       // close librt if there is no monotonic clock
     nits - 'close' -> 'Close'
          - needs period at end of the sentence

 L1499:   // 2. Check for pthread_condattr_setclock support
 L1503:   // libpthread is already loaded
 L1511:   // Now do general initialization
     nit - needs period at end of the sentence

 L1591:   if (timeout < 0)
 L1592:     timeout = 0;
     nit - missing braces

 L1609:       // More seconds than we can add, so pin to max_secs
 L1658:         // More seconds than we can add, so pin to max_secs
     nit - needs period at end of the sentence

 L1643:         // Absolue seconds exceeds allow max, so pin to max_secs
     typo - 'Absolue' -> 'Absolute'
     nit - needs period at end of the sentence

src/os/posix/vm/os_posix.hpp L149: ~PlatformEvent() { guarantee(0, "invariant"); } L185: ~PlatformParker() { guarantee(0, "invariant"); } nit - '0' should be 'false' or just call fatal()

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

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

As Robbin said, this is very hard to review and be sure that everything is relocated correctly. I tried to look at this code a couple of different ways and nothing jumped out at me as wrong.

I did my usual crawl style review through posix.cpp and posix.hpp. I only found nits and typos that you can chose to ignore since you're on a time crunch here.

Thumbs up!

Dan

First a big thank you to Thomas Stuefe for testing various versions of this on AIX. This is primarily a refactoring and cleanup exercise (ie lots of deleted duplicated code!). I have taken the PlatformEvent, PlatformParker and Parker::* code, out of oslinux and moved it into osposix for use by Linux, OSX, BSD, AIX and perhaps one day Solaris (more on that later). The Linux code was the most functionally complete, dealing with correct use of CLOCKMONOTONIC for relative timed waits, and the default wall-clock for absolute timed waits. That functionality is not, unfortunately, supported by all our POSIX platforms so there are some configure time build checks to set some #defines, and then some dynamic lookup at runtime**. We allow for the runtime environment to be less capable than the build environment, but not the other way around (without build time support we don't know the runtime types needed to make library calls). ** There is some duplication of dynamic lookup code on Linux but this can be cleaned up in future work if we refactor the time/clock code into osposix as well. The cleanup covers a number of things: - removal of linux anachronisms that got "ported" into the other platforms - eg EINTR can not be returned from the wait methods - removal of solaris anachronisms that got ported into the linux code and then on to other platforms - eg ETIMEDOUT is what we expect never ETIME - removal of the ancient/obsolete os::*::allowdebugblockedsignals() from the Parker methods - consolidation of unpackTime and computeabstime into one utility function - use statics for things completely private to the implementation rather than making them part of the os* API (eg access to condAttr objects) - cleanup up commentary and style within methods of the same class - clean up coding style in places eg not using Names that start with capitals. I have not tried to cleanup every single oddity, nor tried to reconcile differences between the very similar in places PlatformEvent and Park methods. For example PlatformEvent still examines the FilterSpuriousWakeups** flag, and Parker still ignores it. ** Perhaps a candidate for deprecation and future removal. There is one mini "enhancement" slipped in this. I now explicitly initialize mutexes with a mutexAttr object with its type set to PTHREADMUTEXNORMAL, instead of relying on the definition of PTHREADMUTEXDEFAULT. On FreesBSD the default is not "normal" but "error checking" and so is slow. On all other current platforms there is no effective change. Finally, Solaris is excluded from all this (other than the debug signal blocking cleanup) because it potentially supports three different low-level sync subsystems: UI thr*, Pthread, and direct LWP sync. Solaris cleanup would be a separate RFE. No doubt I've overlooked mentioning something that someone will spot. :) Thanks, David

More information about the hotspot-dev mailing list