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

[David Holmes]

<dropped build-dev>

Hi Dan,

Thanks very much for the review. I will apply all the (mostly inherited) 
grammatical fixes to the comments, etc.

What do you think about Robbin's suggested refactoring of the to_abstime 
logic? (http://cr.openjdk.java.net/~rehn/8174231/webrev/)

Thanks,
David

On 26/05/2017 2:48 AM, Daniel D. Daugherty wrote:
> 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
>>
>