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
