(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
David Holmes
david.holmes at oracle.com
Fri May 26 02:29:36 UTC 2017
On 26/05/2017 12:24 PM, Daniel D. Daugherty wrote:
> On 5/25/17 6:39 PM, David Holmes wrote:
>> <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/)
>
> It's cleaner code. It will make it more difficult to compare against
> the original, but one could easily argue that it's very difficult to
> compare the os_posix.[ch]pp code with the originals...
Okay I'll make the change. I still want to run some more tests anyway.
Thanks,
David
> Dan
>
>
>>
>> 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
>>>>
>>>
>
More information about the hotspot-dev
mailing list