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


Reply via email to