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

[Daniel D. Daugherty]

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):
   - Sometimes you've updated the copyright year for the file and
     sometimes you haven't. Please check before pushing.


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 os_linux and moved it into os_posix 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 CLOCK_MONOTONIC 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 os_posix 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::*::allowdebug_blocked_signals() 
> from the Parker methods
> - consolidation of unpackTime and compute_abstime 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 
> PTHREAD_MUTEX_NORMAL, instead of relying on the definition of 
> PTHREAD_MUTEX_DEFAULT. 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
>