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

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Thanks Erik!

David

On 19/05/2017 6:07 PM, Erik Joelsson wrote:

> Build changes look good to me.
>
> /Erik
>
>
> On 2017-05-19 09:15, David Holmes wrote:
>> Hi Magnus,
>>
>> On 18/05/2017 8:06 PM, Magnus Ihse Bursie wrote:
>>>
>>>
>>> On 2017-05-18 09:35, David Holmes wrote:
>>>> On 18/05/2017 5:32 PM, Magnus Ihse Bursie wrote:
>>>>> On 2017-05-18 08:25, David Holmes wrote:
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8174231
>>>>>>
>>>>>> webrevs:
>>>>>>
>>>>>> Build-related:
>>>>>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
>>>>>
>>>>> Build changes look  good.
>>>>
>>>> Thanks Magnus! I just realized I left in the AC_MSG_NOTICE debugging
>>>> prints outs - do you want me to remove them? I suppose they may be
>>>> useful if something goes wrong on some platform.
>>>
>>> I didn't even notice them. :-/
>>>
>>> It's a bit unfortunate we don't have a debug level on the logging
>>> from configure. :-( Otherwise they would have clearly belonged there.
>>>
>>> The AC_MSG_NOTICE messages stands out much from the rest of the
>>> configure log, so maybe it's better that you remove them. The logic
>>> itself is very simple, if the -D flags are missing then we can surely
>>> tell what happened. So yes, please remove them.
>>
>> Webrev updated in place.
>>
>> I have removed them to avoid noise - particularly as they get executed
>> twice.
>>
>> I also made an adjustment to AC_SEARCH_LIBS as I don't want to pass
>> the saved_LIBS value in explicitly, but want it to use the LIBS value
>> - which is no longer cleared before the call. I've verified all
>> platforms are okay - except AIX which I'll need Thomas to recheck when
>> he can.
>>
>> I also discovered an oddity in that our ARM64 builds seem to use
>> different system libraries in that librt.so is not needed for
>> clock_gettime. This still seems to work ok. Of more of a concern if we
>> were expand this kind of function-existence check is that libc seems
>> to contain "dummy" (or at least dysfunctional) versions of a number of
>> the core pthread APIs!
>>
>> Thanks,
>> David
>>
>>> Alternatively, rewrite them as CHECKING/RESULT, if you want to keep
>>> the logging. That way they match better the rest of the configure log
>>> (and also describes what you're doing). Just check if AC_SEARCH_LIBS
>>> prints some output (likely so, I think), then you can't do it in the
>>> middle of a CHECKING/RESULT pair, but have to do the CHECKING part
>>> after AC_SEARCH_LIBS.
>>>
>>> /Magnus
>>>
>>>>
>>>> David
>>>>
>>>>> /Magnus
>>>>>
>>>>>>       hotspot:
>>>>>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/
>>>>>>
>>>>>> 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 | Threaded
Open this post in threaded view
|

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

David Holmes
<dropping build-dev>

Hi Robbin,

On 19/05/2017 10:33 PM, Robbin Ehn wrote:

> Hi David
>
> On 05/19/2017 01:36 PM, David Holmes wrote:
>>>
>>> There are three different forms of the calculation. The two relative
>>> time versions use a different time function and so a different time
>>> structure (timeval vs timespec) and a different calculation.
>
> Yes that's why I included unit in my example signature.
>
>>
>> It's more complicated than that because we may have build time
>> SUPPORTS_CLOCK_MONOTONIC but we still need the runtime check as well.
>>
>> to_abstime is the old linux unpackTime with the addition of the build
>> time conditionals.
>
> I'm not changing that, I'm not sure how to explain better,
> so here is the patch (only compiled and silly naming convention):
> http://cr.openjdk.java.net/~rehn/8174231/webrev/

Thanks for the taking the time to do this.

> This makes the calculation independent of source/unit.

Okay I see. Took me a few read throughs to get the gist of it - and it
helps to read from the bottom functions up :)

Not sure why you are returning a value from the functions though ??

Let's see what others think. It's somewhat harder to compare against the
existing code.

Thanks again.
David

> Thanks!
>
> /Robbin
>
>
>
>>
>> David
>>
>>> David
>>> -----
>>>
>>>> I do not see a problem with this, only better readability?
>>>>
>>>> /Robbin
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>       struct timespec now;
>>>>>>       int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>>       assert_status(status == 0, status, "clock_gettime");
>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec,
>>>>>> now.tv_nsec, NANOUNITS);
>>>>>>    } else {
>>>>>> #else
>>>>>>    {
>>>>>> #endif
>>>>>>       struct timeval now;
>>>>>>       int status = gettimeofday(&now, NULL);
>>>>>>       assert(status == 0, "gettimeofday");
>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec,
>>>>>> now.tv_usec, MICROUNITS);
>>>>>>    }
>>>>>> #endif
>>>>>>
>>>>>> Thanks for fixing this!
>>>>>>
>>>>>> /Robbin
Reply | Threaded
Open this post in threaded view
|

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

Robbin Ehn
Hi David,

On 05/20/2017 03:07 PM, David Holmes wrote:
> Okay I see. Took me a few read throughs to get the gist of it - and it helps to read from the bottom functions up :)

Great! Yes, C-style with static functions tends to end up that way, since you don't want a lot of forward declarations.

> Not sure why you are returning a value from the functions though ??

I skipped (re-)moving an assert on the max_secs value,
1660   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
just to make the code the same.
So there are some minors/nits in the patch.

>
> Let's see what others think. It's somewhat harder to compare against the existing code.

Yes agreed.

/Robbin

>
> Thanks again.
> David
>
>> Thanks!
>>
>> /Robbin
>>
>>
>>
>>>
>>> David
>>>
>>>> David
>>>> -----
>>>>
>>>>> I do not see a problem with this, only better readability?
>>>>>
>>>>> /Robbin
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>>       struct timespec now;
>>>>>>>       int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>>>       assert_status(status == 0, status, "clock_gettime");
>>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_nsec, NANOUNITS);
>>>>>>>    } else {
>>>>>>> #else
>>>>>>>    {
>>>>>>> #endif
>>>>>>>       struct timeval now;
>>>>>>>       int status = gettimeofday(&now, NULL);
>>>>>>>       assert(status == 0, "gettimeofday");
>>>>>>>       calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>>>>    }
>>>>>>> #endif
>>>>>>>
>>>>>>> Thanks for fixing this!
>>>>>>>
>>>>>>> /Robbin
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by 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
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
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...

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
>>>
>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Robbin, Dan,

Below is a modified version of the refactored to_abstime code that
Robbin suggested.

Robbin: there were a couple of issues with your version. For relative
time the timeout is always in nanoseconds - the "unit" only tells you
what form the "now_part_sec" is - nanos or micros. And the calc_abs_time
always has a deadline in millis. So I simplified and did a little
renaming, and tracked max_secs in debug_only instead of returning it.

Please let me know what you think.

Thanks,
David
-----


// Calculate a new absolute time that is "timeout" nanoseconds from "now".
// "unit" indicates the unit of "now_part_sec" (may be nanos or micros
depending
// on which clock is being used).
static void calc_rel_time(timespec* abstime, jlong timeout, jlong now_sec,
                           jlong now_part_sec, jlong unit) {
   time_t max_secs = now_sec + MAX_SECS;

   jlong seconds = timeout / NANOUNITS;
   timeout %= NANOUNITS; // remaining nanos

   if (seconds >= MAX_SECS) {
     // More seconds than we can add, so pin to max_secs.
     abstime->tv_sec = max_secs;
     abstime->tv_nsec = 0;
   } else {
     abstime->tv_sec = now_sec  + seconds;
     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
     if (nanos >= NANOUNITS) { // overflow
       abstime->tv_sec += 1;
       nanos -= NANOUNITS;
     }
     abstime->tv_nsec = nanos;
   }
}

// Unpack the given deadline in milliseconds since the epoch, into the
given timespec.
// The current time in seconds is also passed in to enforce an upper
bound as discussed above.
static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
now_sec) {
   time_t max_secs = now_sec + MAX_SECS;

   jlong seconds = deadline / MILLIUNITS;
   jlong millis = deadline % MILLIUNITS;

   if (seconds >= max_secs) {
     // Absolute seconds exceeds allowed max, so pin to max_secs.
     abstime->tv_sec = max_secs;
     abstime->tv_nsec = 0;
   } else {
     abstime->tv_sec = seconds;
     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
   }
}


static void to_abstime(timespec* abstime, jlong timeout, bool isAbsolute) {

   DEBUG_ONLY(int max_secs = MAX_SECS;)

   if (timeout < 0) {
     timeout = 0;
   }

#ifdef SUPPORTS_CLOCK_MONOTONIC

   if (_use_clock_monotonic_condattr && !isAbsolute) {
     struct timespec now;
     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
     assert_status(status == 0, status, "clock_gettime");
     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
     DEBUG_ONLY(max_secs += now.tv_sec;)
   } else {

#else

   { // Match the block scope.

#endif // SUPPORTS_CLOCK_MONOTONIC

     // Time-of-day clock is all we can reliably use.
     struct timeval now;
     int status = gettimeofday(&now, NULL);
     assert(status == 0, "gettimeofday");
     if (isAbsolute) {
       unpack_abs_time(abstime, timeout, now.tv_sec);
     }
     else {
       calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
     }
     DEBUG_ONLY(max_secs += now.tv_sec;)
   }

   assert(abstime->tv_sec >= 0, "tv_sec < 0");
   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >= nanos_per_sec");

}
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
On 5/26/17 1:27 AM, David Holmes wrote:

> Robbin, Dan,
>
> Below is a modified version of the refactored to_abstime code that
> Robbin suggested.
>
> Robbin: there were a couple of issues with your version. For relative
> time the timeout is always in nanoseconds - the "unit" only tells you
> what form the "now_part_sec" is - nanos or micros. And the
> calc_abs_time always has a deadline in millis. So I simplified and did
> a little renaming, and tracked max_secs in debug_only instead of
> returning it.
>
> Please let me know what you think.

Looks OK to me. Nit comments below...


>
> Thanks,
> David
> -----
>
>
> // Calculate a new absolute time that is "timeout" nanoseconds from
> "now".
> // "unit" indicates the unit of "now_part_sec" (may be nanos or micros
> depending
> // on which clock is being used).
> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
> now_sec,
>                           jlong now_part_sec, jlong unit) {
>   time_t max_secs = now_sec + MAX_SECS;
>
>   jlong seconds = timeout / NANOUNITS;
>   timeout %= NANOUNITS; // remaining nanos
>
>   if (seconds >= MAX_SECS) {
>     // More seconds than we can add, so pin to max_secs.
>     abstime->tv_sec = max_secs;
>     abstime->tv_nsec = 0;
>   } else {
>     abstime->tv_sec = now_sec  + seconds;
>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>     if (nanos >= NANOUNITS) { // overflow
>       abstime->tv_sec += 1;
>       nanos -= NANOUNITS;
>     }
>     abstime->tv_nsec = nanos;
>   }
> }
>
> // Unpack the given deadline in milliseconds since the epoch, into the
> given timespec.
> // The current time in seconds is also passed in to enforce an upper
> bound as discussed above.
> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
> now_sec) {
>   time_t max_secs = now_sec + MAX_SECS;
>
>   jlong seconds = deadline / MILLIUNITS;
>   jlong millis = deadline % MILLIUNITS;
>
>   if (seconds >= max_secs) {
>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>     abstime->tv_sec = max_secs;
>     abstime->tv_nsec = 0;
>   } else {
>     abstime->tv_sec = seconds;
>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>   }
> }
>
>
> static void to_abstime(timespec* abstime, jlong timeout, bool
> isAbsolute) {

There's an extra blank line here.

>
>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>
>   if (timeout < 0) {
>     timeout = 0;
>   }
>
> #ifdef SUPPORTS_CLOCK_MONOTONIC
>
>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>     struct timespec now;
>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>     assert_status(status == 0, status, "clock_gettime");
>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>     DEBUG_ONLY(max_secs += now.tv_sec;)
>   } else {
>
> #else
>
>   { // Match the block scope.
>
> #endif // SUPPORTS_CLOCK_MONOTONIC
>
>     // Time-of-day clock is all we can reliably use.
>     struct timeval now;
>     int status = gettimeofday(&now, NULL);
>     assert(status == 0, "gettimeofday");

assert_status() is used above, but assert() is used here. Why?


>     if (isAbsolute) {
>       unpack_abs_time(abstime, timeout, now.tv_sec);
>     }
>     else {

Inconsistent "else-branch" formatting.
I believe HotSpot style is "} else {"


> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>     }
>     DEBUG_ONLY(max_secs += now.tv_sec;)
>   }
>
>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
> nanos_per_sec");

Why does the assert mesg have "nanos_per_sec" instead of "NANOSECS_PER_SEC"?

There's an extra blank line here.

>
> }

Definitely looks and reads much cleaner.

Dan

Reply | Threaded
Open this post in threaded view
|

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

Robbin Ehn
Thanks David, looks good!

/Robbin

On 2017-05-26 20:19, Daniel D. Daugherty wrote:

> On 5/26/17 1:27 AM, David Holmes wrote:
>> Robbin, Dan,
>>
>> Below is a modified version of the refactored to_abstime code that
>> Robbin suggested.
>>
>> Robbin: there were a couple of issues with your version. For relative
>> time the timeout is always in nanoseconds - the "unit" only tells you
>> what form the "now_part_sec" is - nanos or micros. And the
>> calc_abs_time always has a deadline in millis. So I simplified and did
>> a little renaming, and tracked max_secs in debug_only instead of
>> returning it.
>>
>> Please let me know what you think.
>
> Looks OK to me. Nit comments below...
>
>
>>
>> Thanks,
>> David
>> -----
>>
>>
>> // Calculate a new absolute time that is "timeout" nanoseconds from
>> "now".
>> // "unit" indicates the unit of "now_part_sec" (may be nanos or micros
>> depending
>> // on which clock is being used).
>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>> now_sec,
>>                           jlong now_part_sec, jlong unit) {
>>   time_t max_secs = now_sec + MAX_SECS;
>>
>>   jlong seconds = timeout / NANOUNITS;
>>   timeout %= NANOUNITS; // remaining nanos
>>
>>   if (seconds >= MAX_SECS) {
>>     // More seconds than we can add, so pin to max_secs.
>>     abstime->tv_sec = max_secs;
>>     abstime->tv_nsec = 0;
>>   } else {
>>     abstime->tv_sec = now_sec  + seconds;
>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>     if (nanos >= NANOUNITS) { // overflow
>>       abstime->tv_sec += 1;
>>       nanos -= NANOUNITS;
>>     }
>>     abstime->tv_nsec = nanos;
>>   }
>> }
>>
>> // Unpack the given deadline in milliseconds since the epoch, into the
>> given timespec.
>> // The current time in seconds is also passed in to enforce an upper
>> bound as discussed above.
>> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
>> now_sec) {
>>   time_t max_secs = now_sec + MAX_SECS;
>>
>>   jlong seconds = deadline / MILLIUNITS;
>>   jlong millis = deadline % MILLIUNITS;
>>
>>   if (seconds >= max_secs) {
>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>     abstime->tv_sec = max_secs;
>>     abstime->tv_nsec = 0;
>>   } else {
>>     abstime->tv_sec = seconds;
>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>   }
>> }
>>
>>
>> static void to_abstime(timespec* abstime, jlong timeout, bool
>> isAbsolute) {
>
> There's an extra blank line here.
>
>>
>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>
>>   if (timeout < 0) {
>>     timeout = 0;
>>   }
>>
>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>
>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>     struct timespec now;
>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>     assert_status(status == 0, status, "clock_gettime");
>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>   } else {
>>
>> #else
>>
>>   { // Match the block scope.
>>
>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>
>>     // Time-of-day clock is all we can reliably use.
>>     struct timeval now;
>>     int status = gettimeofday(&now, NULL);
>>     assert(status == 0, "gettimeofday");
>
> assert_status() is used above, but assert() is used here. Why?
>
>
>>     if (isAbsolute) {
>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>     }
>>     else {
>
> Inconsistent "else-branch" formatting.
> I believe HotSpot style is "} else {"
>
>
>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>     }
>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>   }
>>
>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>> nanos_per_sec");
>
> Why does the assert mesg have "nanos_per_sec" instead of
> "NANOSECS_PER_SEC"?
>
> There's an extra blank line here.
>
>>
>> }
>
> Definitely looks and reads much cleaner.
>
> Dan
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Daniel D. Daugherty
On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:

> On 5/26/17 1:27 AM, David Holmes wrote:
>> Robbin, Dan,
>>
>> Below is a modified version of the refactored to_abstime code that
>> Robbin suggested.
>>
>> Robbin: there were a couple of issues with your version. For relative
>> time the timeout is always in nanoseconds - the "unit" only tells you
>> what form the "now_part_sec" is - nanos or micros. And the
>> calc_abs_time always has a deadline in millis. So I simplified and did
>> a little renaming, and tracked max_secs in debug_only instead of
>> returning it.
>>
>> Please let me know what you think.
>
> Looks OK to me. Nit comments below...

Thanks Dan - more below.

>>
>>
>> // Calculate a new absolute time that is "timeout" nanoseconds from
>> "now".
>> // "unit" indicates the unit of "now_part_sec" (may be nanos or micros
>> depending
>> // on which clock is being used).
>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>> now_sec,
>>                           jlong now_part_sec, jlong unit) {
>>   time_t max_secs = now_sec + MAX_SECS;
>>
>>   jlong seconds = timeout / NANOUNITS;
>>   timeout %= NANOUNITS; // remaining nanos
>>
>>   if (seconds >= MAX_SECS) {
>>     // More seconds than we can add, so pin to max_secs.
>>     abstime->tv_sec = max_secs;
>>     abstime->tv_nsec = 0;
>>   } else {
>>     abstime->tv_sec = now_sec  + seconds;
>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>     if (nanos >= NANOUNITS) { // overflow
>>       abstime->tv_sec += 1;
>>       nanos -= NANOUNITS;
>>     }
>>     abstime->tv_nsec = nanos;
>>   }
>> }
>>
>> // Unpack the given deadline in milliseconds since the epoch, into the
>> given timespec.
>> // The current time in seconds is also passed in to enforce an upper
>> bound as discussed above.
>> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
>> now_sec) {
>>   time_t max_secs = now_sec + MAX_SECS;
>>
>>   jlong seconds = deadline / MILLIUNITS;
>>   jlong millis = deadline % MILLIUNITS;
>>
>>   if (seconds >= max_secs) {
>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>     abstime->tv_sec = max_secs;
>>     abstime->tv_nsec = 0;
>>   } else {
>>     abstime->tv_sec = seconds;
>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>   }
>> }
>>
>>
>> static void to_abstime(timespec* abstime, jlong timeout, bool
>> isAbsolute) {
>
> There's an extra blank line here.

Fixed.

>>
>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>
>>   if (timeout < 0) {
>>     timeout = 0;
>>   }
>>
>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>
>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>     struct timespec now;
>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>     assert_status(status == 0, status, "clock_gettime");
>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>   } else {
>>
>> #else
>>
>>   { // Match the block scope.
>>
>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>
>>     // Time-of-day clock is all we can reliably use.
>>     struct timeval now;
>>     int status = gettimeofday(&now, NULL);
>>     assert(status == 0, "gettimeofday");
>
> assert_status() is used above, but assert() is used here. Why?

Historical. assert_status was introduced for the pthread* and other
posix funcs that return the error value rather than returning -1 and
setting errno. gettimeofday is not one of those so still has the old
assert. However, as someone pointed out a while ago you can use
assert_status with these and pass errno as the "status". So I did that.

>
>>     if (isAbsolute) {
>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>     }
>>     else {
>
> Inconsistent "else-branch" formatting.
> I believe HotSpot style is "} else {"

Fixed.

>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>     }
>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>   }
>>
>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>> nanos_per_sec");
>
> Why does the assert mesg have "nanos_per_sec" instead of
> "NANOSECS_PER_SEC"?

No reason. Actually that should now refer to NANOUNITS. Hmmm I can not
recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly an
oversight.

> There's an extra blank line here.

Fixed.

Will send out complete updated webrev soon.

Thanks,
David

>>
>> }
>
> Definitely looks and reads much cleaner.
>
> Dan
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Dan, Robbin, Thomas,

Okay here is the final ready to push version:

http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/

this fixes all Dan's nits and refactors the time calculation code as
suggested by Robbin.

Thomas: if you are around and able, it would be good to get a final
sanity check on AIX. Thanks.

Testing:
  - JPRT: -testset hotspot
          -testset core

- manual:
    - jtreg:java/util/concurrent
    - various little test programs that try to validate sleep/wait times
to show early returns or unexpected delays

Thanks again for the reviews.

David

On 29/05/2017 10:29 AM, David Holmes wrote:

> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>> On 5/26/17 1:27 AM, David Holmes wrote:
>>> Robbin, Dan,
>>>
>>> Below is a modified version of the refactored to_abstime code that
>>> Robbin suggested.
>>>
>>> Robbin: there were a couple of issues with your version. For relative
>>> time the timeout is always in nanoseconds - the "unit" only tells you
>>> what form the "now_part_sec" is - nanos or micros. And the
>>> calc_abs_time always has a deadline in millis. So I simplified and
>>> did a little renaming, and tracked max_secs in debug_only instead of
>>> returning it.
>>>
>>> Please let me know what you think.
>>
>> Looks OK to me. Nit comments below...
>
> Thanks Dan - more below.
>
>>>
>>>
>>> // Calculate a new absolute time that is "timeout" nanoseconds from
>>> "now".
>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or
>>> micros depending
>>> // on which clock is being used).
>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>>> now_sec,
>>>                           jlong now_part_sec, jlong unit) {
>>>   time_t max_secs = now_sec + MAX_SECS;
>>>
>>>   jlong seconds = timeout / NANOUNITS;
>>>   timeout %= NANOUNITS; // remaining nanos
>>>
>>>   if (seconds >= MAX_SECS) {
>>>     // More seconds than we can add, so pin to max_secs.
>>>     abstime->tv_sec = max_secs;
>>>     abstime->tv_nsec = 0;
>>>   } else {
>>>     abstime->tv_sec = now_sec  + seconds;
>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>     if (nanos >= NANOUNITS) { // overflow
>>>       abstime->tv_sec += 1;
>>>       nanos -= NANOUNITS;
>>>     }
>>>     abstime->tv_nsec = nanos;
>>>   }
>>> }
>>>
>>> // Unpack the given deadline in milliseconds since the epoch, into
>>> the given timespec.
>>> // The current time in seconds is also passed in to enforce an upper
>>> bound as discussed above.
>>> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
>>> now_sec) {
>>>   time_t max_secs = now_sec + MAX_SECS;
>>>
>>>   jlong seconds = deadline / MILLIUNITS;
>>>   jlong millis = deadline % MILLIUNITS;
>>>
>>>   if (seconds >= max_secs) {
>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>     abstime->tv_sec = max_secs;
>>>     abstime->tv_nsec = 0;
>>>   } else {
>>>     abstime->tv_sec = seconds;
>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>   }
>>> }
>>>
>>>
>>> static void to_abstime(timespec* abstime, jlong timeout, bool
>>> isAbsolute) {
>>
>> There's an extra blank line here.
>
> Fixed.
>
>>>
>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>
>>>   if (timeout < 0) {
>>>     timeout = 0;
>>>   }
>>>
>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>
>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>     struct timespec now;
>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>     assert_status(status == 0, status, "clock_gettime");
>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>   } else {
>>>
>>> #else
>>>
>>>   { // Match the block scope.
>>>
>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>
>>>     // Time-of-day clock is all we can reliably use.
>>>     struct timeval now;
>>>     int status = gettimeofday(&now, NULL);
>>>     assert(status == 0, "gettimeofday");
>>
>> assert_status() is used above, but assert() is used here. Why?
>
> Historical. assert_status was introduced for the pthread* and other
> posix funcs that return the error value rather than returning -1 and
> setting errno. gettimeofday is not one of those so still has the old
> assert. However, as someone pointed out a while ago you can use
> assert_status with these and pass errno as the "status". So I did that.
>
>>
>>>     if (isAbsolute) {
>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>     }
>>>     else {
>>
>> Inconsistent "else-branch" formatting.
>> I believe HotSpot style is "} else {"
>
> Fixed.
>
>>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>     }
>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>   }
>>>
>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>>> nanos_per_sec");
>>
>> Why does the assert mesg have "nanos_per_sec" instead of
>> "NANOSECS_PER_SEC"?
>
> No reason. Actually that should now refer to NANOUNITS. Hmmm I can not
> recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly an
> oversight.
>
>> There's an extra blank line here.
>
> Fixed.
>
> Will send out complete updated webrev soon.
>
> Thanks,
> David
>
>>>
>>> }
>>
>> Definitely looks and reads much cleaner.
>>
>> Dan
>>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
Hi David,

works fine on AIX, as it did before.

Had a look at the change, some very small nits:

- We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
pthread_cond_t). Are the rules not that each header should be
self-contained? If yes, should os_posix.hpp not include the relevant OS
headers for the pthread_... types?

- the coding around dlopen/dlsym would be a bit more readable if you were
to define types for the function pointers, that would save you from
spelling them out each time.

e.g.:

typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
...
static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
dlsym(...);

...Thomas


On Mon, May 29, 2017 at 6:19 AM, David Holmes <[hidden email]>
wrote:

> Dan, Robbin, Thomas,
>
> Okay here is the final ready to push version:
>
> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>
> this fixes all Dan's nits and refactors the time calculation code as
> suggested by Robbin.
>
> Thomas: if you are around and able, it would be good to get a final sanity
> check on AIX. Thanks.
>
> Testing:
>  - JPRT: -testset hotspot
>          -testset core
>
> - manual:
>    - jtreg:java/util/concurrent
>    - various little test programs that try to validate sleep/wait times to
> show early returns or unexpected delays
>
> Thanks again for the reviews.
>
> David
>
>
> On 29/05/2017 10:29 AM, David Holmes wrote:
>
>> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>
>>> On 5/26/17 1:27 AM, David Holmes wrote:
>>>
>>>> Robbin, Dan,
>>>>
>>>> Below is a modified version of the refactored to_abstime code that
>>>> Robbin suggested.
>>>>
>>>> Robbin: there were a couple of issues with your version. For relative
>>>> time the timeout is always in nanoseconds - the "unit" only tells you what
>>>> form the "now_part_sec" is - nanos or micros. And the calc_abs_time always
>>>> has a deadline in millis. So I simplified and did a little renaming, and
>>>> tracked max_secs in debug_only instead of returning it.
>>>>
>>>> Please let me know what you think.
>>>>
>>>
>>> Looks OK to me. Nit comments below...
>>>
>>
>> Thanks Dan - more below.
>>
>>
>>>>
>>>> // Calculate a new absolute time that is "timeout" nanoseconds from
>>>> "now".
>>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or micros
>>>> depending
>>>> // on which clock is being used).
>>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>>>> now_sec,
>>>>                           jlong now_part_sec, jlong unit) {
>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>   jlong seconds = timeout / NANOUNITS;
>>>>   timeout %= NANOUNITS; // remaining nanos
>>>>
>>>>   if (seconds >= MAX_SECS) {
>>>>     // More seconds than we can add, so pin to max_secs.
>>>>     abstime->tv_sec = max_secs;
>>>>     abstime->tv_nsec = 0;
>>>>   } else {
>>>>     abstime->tv_sec = now_sec  + seconds;
>>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>>     if (nanos >= NANOUNITS) { // overflow
>>>>       abstime->tv_sec += 1;
>>>>       nanos -= NANOUNITS;
>>>>     }
>>>>     abstime->tv_nsec = nanos;
>>>>   }
>>>> }
>>>>
>>>> // Unpack the given deadline in milliseconds since the epoch, into the
>>>> given timespec.
>>>> // The current time in seconds is also passed in to enforce an upper
>>>> bound as discussed above.
>>>> static void unpack_abs_time(timespec* abstime, jlong deadline, jlong
>>>> now_sec) {
>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>   jlong seconds = deadline / MILLIUNITS;
>>>>   jlong millis = deadline % MILLIUNITS;
>>>>
>>>>   if (seconds >= max_secs) {
>>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>>     abstime->tv_sec = max_secs;
>>>>     abstime->tv_nsec = 0;
>>>>   } else {
>>>>     abstime->tv_sec = seconds;
>>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>>   }
>>>> }
>>>>
>>>>
>>>> static void to_abstime(timespec* abstime, jlong timeout, bool
>>>> isAbsolute) {
>>>>
>>>
>>> There's an extra blank line here.
>>>
>>
>> Fixed.
>>
>>
>>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>>
>>>>   if (timeout < 0) {
>>>>     timeout = 0;
>>>>   }
>>>>
>>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>>     struct timespec now;
>>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>     assert_status(status == 0, status, "clock_gettime");
>>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec, NANOUNITS);
>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>   } else {
>>>>
>>>> #else
>>>>
>>>>   { // Match the block scope.
>>>>
>>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>     // Time-of-day clock is all we can reliably use.
>>>>     struct timeval now;
>>>>     int status = gettimeofday(&now, NULL);
>>>>     assert(status == 0, "gettimeofday");
>>>>
>>>
>>> assert_status() is used above, but assert() is used here. Why?
>>>
>>
>> Historical. assert_status was introduced for the pthread* and other posix
>> funcs that return the error value rather than returning -1 and setting
>> errno. gettimeofday is not one of those so still has the old assert.
>> However, as someone pointed out a while ago you can use assert_status with
>> these and pass errno as the "status". So I did that.
>>
>>
>>>     if (isAbsolute) {
>>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>>     }
>>>>     else {
>>>>
>>>
>>> Inconsistent "else-branch" formatting.
>>> I believe HotSpot style is "} else {"
>>>
>>
>> Fixed.
>>
>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>     }
>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>   }
>>>>
>>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>>>> nanos_per_sec");
>>>>
>>>
>>> Why does the assert mesg have "nanos_per_sec" instead of
>>> "NANOSECS_PER_SEC"?
>>>
>>
>> No reason. Actually that should now refer to NANOUNITS. Hmmm I can not
>> recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly an
>> oversight.
>>
>> There's an extra blank line here.
>>>
>>
>> Fixed.
>>
>> Will send out complete updated webrev soon.
>>
>> Thanks,
>> David
>>
>>
>>>> }
>>>>
>>>
>>> Definitely looks and reads much cleaner.
>>>
>>> Dan
>>>
>>>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Thomas,

On 30/05/2017 7:53 PM, Thomas Stüfe wrote:
> Hi David,
>
> works fine on AIX, as it did before.

Great - thanks for confirming that again.

> Had a look at the change, some very small nits:
>
> - We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
> pthread_cond_t). Are the rules not that each header should be
> self-contained? If yes, should os_posix.hpp not include the relevant OS
> headers for the pthread_... types?

Yes. I'll fix that - and check how the pthread types are currently
getting seen. Thanks.

> - the coding around dlopen/dlsym would be a bit more readable if you
> were to define types for the function pointers, that would save you from
> spelling them out each time.

I'll safe this for the next cleanup if we share all of the "clock"
related stuff.

Thanks again,
David
-----

> e.g.:
>
> typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
> ...
> static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
> dlsym(...);
>
> ...Thomas
>
>
> On Mon, May 29, 2017 at 6:19 AM, David Holmes <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Dan, Robbin, Thomas,
>
>     Okay here is the final ready to push version:
>
>     http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>     <http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/>
>
>     this fixes all Dan's nits and refactors the time calculation code as
>     suggested by Robbin.
>
>     Thomas: if you are around and able, it would be good to get a final
>     sanity check on AIX. Thanks.
>
>     Testing:
>       - JPRT: -testset hotspot
>               -testset core
>
>     - manual:
>         - jtreg:java/util/concurrent
>         - various little test programs that try to validate sleep/wait
>     times to show early returns or unexpected delays
>
>     Thanks again for the reviews.
>
>     David
>
>
>     On 29/05/2017 10:29 AM, David Holmes wrote:
>
>         On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>
>             On 5/26/17 1:27 AM, David Holmes wrote:
>
>                 Robbin, Dan,
>
>                 Below is a modified version of the refactored to_abstime
>                 code that Robbin suggested.
>
>                 Robbin: there were a couple of issues with your version.
>                 For relative time the timeout is always in nanoseconds -
>                 the "unit" only tells you what form the "now_part_sec"
>                 is - nanos or micros. And the calc_abs_time always has a
>                 deadline in millis. So I simplified and did a little
>                 renaming, and tracked max_secs in debug_only instead of
>                 returning it.
>
>                 Please let me know what you think.
>
>
>             Looks OK to me. Nit comments below...
>
>
>         Thanks Dan - more below.
>
>
>
>                 // Calculate a new absolute time that is "timeout"
>                 nanoseconds from "now".
>                 // "unit" indicates the unit of "now_part_sec" (may be
>                 nanos or micros depending
>                 // on which clock is being used).
>                 static void calc_rel_time(timespec* abstime, jlong
>                 timeout, jlong now_sec,
>                                            jlong now_part_sec, jlong unit) {
>                    time_t max_secs = now_sec + MAX_SECS;
>
>                    jlong seconds = timeout / NANOUNITS;
>                    timeout %= NANOUNITS; // remaining nanos
>
>                    if (seconds >= MAX_SECS) {
>                      // More seconds than we can add, so pin to max_secs.
>                      abstime->tv_sec = max_secs;
>                      abstime->tv_nsec = 0;
>                    } else {
>                      abstime->tv_sec = now_sec  + seconds;
>                      long nanos = (now_part_sec * (NANOUNITS / unit)) +
>                 timeout;
>                      if (nanos >= NANOUNITS) { // overflow
>                        abstime->tv_sec += 1;
>                        nanos -= NANOUNITS;
>                      }
>                      abstime->tv_nsec = nanos;
>                    }
>                 }
>
>                 // Unpack the given deadline in milliseconds since the
>                 epoch, into the given timespec.
>                 // The current time in seconds is also passed in to
>                 enforce an upper bound as discussed above.
>                 static void unpack_abs_time(timespec* abstime, jlong
>                 deadline, jlong now_sec) {
>                    time_t max_secs = now_sec + MAX_SECS;
>
>                    jlong seconds = deadline / MILLIUNITS;
>                    jlong millis = deadline % MILLIUNITS;
>
>                    if (seconds >= max_secs) {
>                      // Absolute seconds exceeds allowed max, so pin to
>                 max_secs.
>                      abstime->tv_sec = max_secs;
>                      abstime->tv_nsec = 0;
>                    } else {
>                      abstime->tv_sec = seconds;
>                      abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>                    }
>                 }
>
>
>                 static void to_abstime(timespec* abstime, jlong timeout,
>                 bool isAbsolute) {
>
>
>             There's an extra blank line here.
>
>
>         Fixed.
>
>
>                    DEBUG_ONLY(int max_secs = MAX_SECS;)
>
>                    if (timeout < 0) {
>                      timeout = 0;
>                    }
>
>                 #ifdef SUPPORTS_CLOCK_MONOTONIC
>
>                    if (_use_clock_monotonic_condattr && !isAbsolute) {
>                      struct timespec now;
>                      int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>                      assert_status(status == 0, status, "clock_gettime");
>                      calc_rel_time(abstime, timeout, now.tv_sec,
>                 now.tv_nsec, NANOUNITS);
>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>                    } else {
>
>                 #else
>
>                    { // Match the block scope.
>
>                 #endif // SUPPORTS_CLOCK_MONOTONIC
>
>                      // Time-of-day clock is all we can reliably use.
>                      struct timeval now;
>                      int status = gettimeofday(&now, NULL);
>                      assert(status == 0, "gettimeofday");
>
>
>             assert_status() is used above, but assert() is used here. Why?
>
>
>         Historical. assert_status was introduced for the pthread* and
>         other posix funcs that return the error value rather than
>         returning -1 and setting errno. gettimeofday is not one of those
>         so still has the old assert. However, as someone pointed out a
>         while ago you can use assert_status with these and pass errno as
>         the "status". So I did that.
>
>
>                      if (isAbsolute) {
>                        unpack_abs_time(abstime, timeout, now.tv_sec);
>                      }
>                      else {
>
>
>             Inconsistent "else-branch" formatting.
>             I believe HotSpot style is "} else {"
>
>
>         Fixed.
>
>                 calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec,
>                 MICROUNITS);
>                      }
>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>                    }
>
>                    assert(abstime->tv_sec >= 0, "tv_sec < 0");
>                    assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>                    assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>                    assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec
>                  >= nanos_per_sec");
>
>
>             Why does the assert mesg have "nanos_per_sec" instead of
>             "NANOSECS_PER_SEC"?
>
>
>         No reason. Actually that should now refer to NANOUNITS. Hmmm I
>         can not recall why we have NANOUNITS and NANAOSECS_PER_SEC ...
>         possibly an oversight.
>
>             There's an extra blank line here.
>
>
>         Fixed.
>
>         Will send out complete updated webrev soon.
>
>         Thanks,
>         David
>
>
>                 }
>
>
>             Definitely looks and reads much cleaner.
>
>             Dan
>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Correction ...

On 30/05/2017 9:22 PM, David Holmes wrote:

> Hi Thomas,
>
> On 30/05/2017 7:53 PM, Thomas Stüfe wrote:
>> Hi David,
>>
>> works fine on AIX, as it did before.
>
> Great - thanks for confirming that again.
>
>> Had a look at the change, some very small nits:
>>
>> - We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
>> pthread_cond_t). Are the rules not that each header should be
>> self-contained? If yes, should os_posix.hpp not include the relevant
>> OS headers for the pthread_... types?
>
> Yes. I'll fix that - and check how the pthread types are currently
> getting seen. Thanks.

So ... os_posix.hpp only #includes "runtime/os.hpp" and yet uses dozens
of types defined in numerous other header files! So I could add
pthread.h, but it would look very lonely.

Dan: thoughts?

Cheers,
David

>> - the coding around dlopen/dlsym would be a bit more readable if you
>> were to define types for the function pointers, that would save you
>> from spelling them out each time.
>
> I'll safe this for the next cleanup if we share all of the "clock"
> related stuff.
>
> Thanks again,
> David
> -----
>
>> e.g.:
>>
>> typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
>> ...
>> static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
>> dlsym(...);
>>
>> ...Thomas
>>
>>
>> On Mon, May 29, 2017 at 6:19 AM, David Holmes <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Dan, Robbin, Thomas,
>>
>>     Okay here is the final ready to push version:
>>
>>     http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>>     <http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/>
>>
>>     this fixes all Dan's nits and refactors the time calculation code as
>>     suggested by Robbin.
>>
>>     Thomas: if you are around and able, it would be good to get a final
>>     sanity check on AIX. Thanks.
>>
>>     Testing:
>>       - JPRT: -testset hotspot
>>               -testset core
>>
>>     - manual:
>>         - jtreg:java/util/concurrent
>>         - various little test programs that try to validate sleep/wait
>>     times to show early returns or unexpected delays
>>
>>     Thanks again for the reviews.
>>
>>     David
>>
>>
>>     On 29/05/2017 10:29 AM, David Holmes wrote:
>>
>>         On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>
>>             On 5/26/17 1:27 AM, David Holmes wrote:
>>
>>                 Robbin, Dan,
>>
>>                 Below is a modified version of the refactored to_abstime
>>                 code that Robbin suggested.
>>
>>                 Robbin: there were a couple of issues with your version.
>>                 For relative time the timeout is always in nanoseconds -
>>                 the "unit" only tells you what form the "now_part_sec"
>>                 is - nanos or micros. And the calc_abs_time always has a
>>                 deadline in millis. So I simplified and did a little
>>                 renaming, and tracked max_secs in debug_only instead of
>>                 returning it.
>>
>>                 Please let me know what you think.
>>
>>
>>             Looks OK to me. Nit comments below...
>>
>>
>>         Thanks Dan - more below.
>>
>>
>>
>>                 // Calculate a new absolute time that is "timeout"
>>                 nanoseconds from "now".
>>                 // "unit" indicates the unit of "now_part_sec" (may be
>>                 nanos or micros depending
>>                 // on which clock is being used).
>>                 static void calc_rel_time(timespec* abstime, jlong
>>                 timeout, jlong now_sec,
>>                                            jlong now_part_sec, jlong
>> unit) {
>>                    time_t max_secs = now_sec + MAX_SECS;
>>
>>                    jlong seconds = timeout / NANOUNITS;
>>                    timeout %= NANOUNITS; // remaining nanos
>>
>>                    if (seconds >= MAX_SECS) {
>>                      // More seconds than we can add, so pin to max_secs.
>>                      abstime->tv_sec = max_secs;
>>                      abstime->tv_nsec = 0;
>>                    } else {
>>                      abstime->tv_sec = now_sec  + seconds;
>>                      long nanos = (now_part_sec * (NANOUNITS / unit)) +
>>                 timeout;
>>                      if (nanos >= NANOUNITS) { // overflow
>>                        abstime->tv_sec += 1;
>>                        nanos -= NANOUNITS;
>>                      }
>>                      abstime->tv_nsec = nanos;
>>                    }
>>                 }
>>
>>                 // Unpack the given deadline in milliseconds since the
>>                 epoch, into the given timespec.
>>                 // The current time in seconds is also passed in to
>>                 enforce an upper bound as discussed above.
>>                 static void unpack_abs_time(timespec* abstime, jlong
>>                 deadline, jlong now_sec) {
>>                    time_t max_secs = now_sec + MAX_SECS;
>>
>>                    jlong seconds = deadline / MILLIUNITS;
>>                    jlong millis = deadline % MILLIUNITS;
>>
>>                    if (seconds >= max_secs) {
>>                      // Absolute seconds exceeds allowed max, so pin to
>>                 max_secs.
>>                      abstime->tv_sec = max_secs;
>>                      abstime->tv_nsec = 0;
>>                    } else {
>>                      abstime->tv_sec = seconds;
>>                      abstime->tv_nsec = millis * (NANOUNITS /
>> MILLIUNITS);
>>                    }
>>                 }
>>
>>
>>                 static void to_abstime(timespec* abstime, jlong timeout,
>>                 bool isAbsolute) {
>>
>>
>>             There's an extra blank line here.
>>
>>
>>         Fixed.
>>
>>
>>                    DEBUG_ONLY(int max_secs = MAX_SECS;)
>>
>>                    if (timeout < 0) {
>>                      timeout = 0;
>>                    }
>>
>>                 #ifdef SUPPORTS_CLOCK_MONOTONIC
>>
>>                    if (_use_clock_monotonic_condattr && !isAbsolute) {
>>                      struct timespec now;
>>                      int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>                      assert_status(status == 0, status, "clock_gettime");
>>                      calc_rel_time(abstime, timeout, now.tv_sec,
>>                 now.tv_nsec, NANOUNITS);
>>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>>                    } else {
>>
>>                 #else
>>
>>                    { // Match the block scope.
>>
>>                 #endif // SUPPORTS_CLOCK_MONOTONIC
>>
>>                      // Time-of-day clock is all we can reliably use.
>>                      struct timeval now;
>>                      int status = gettimeofday(&now, NULL);
>>                      assert(status == 0, "gettimeofday");
>>
>>
>>             assert_status() is used above, but assert() is used here.
>> Why?
>>
>>
>>         Historical. assert_status was introduced for the pthread* and
>>         other posix funcs that return the error value rather than
>>         returning -1 and setting errno. gettimeofday is not one of those
>>         so still has the old assert. However, as someone pointed out a
>>         while ago you can use assert_status with these and pass errno as
>>         the "status". So I did that.
>>
>>
>>                      if (isAbsolute) {
>>                        unpack_abs_time(abstime, timeout, now.tv_sec);
>>                      }
>>                      else {
>>
>>
>>             Inconsistent "else-branch" formatting.
>>             I believe HotSpot style is "} else {"
>>
>>
>>         Fixed.
>>
>>                 calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec,
>>                 MICROUNITS);
>>                      }
>>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>>                    }
>>
>>                    assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>                    assert(abstime->tv_sec <= max_secs, "tv_sec >
>> max_secs");
>>                    assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>                    assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec
>>                  >= nanos_per_sec");
>>
>>
>>             Why does the assert mesg have "nanos_per_sec" instead of
>>             "NANOSECS_PER_SEC"?
>>
>>
>>         No reason. Actually that should now refer to NANOUNITS. Hmmm I
>>         can not recall why we have NANOUNITS and NANAOSECS_PER_SEC ...
>>         possibly an oversight.
>>
>>             There's an extra blank line here.
>>
>>
>>         Fixed.
>>
>>         Will send out complete updated webrev soon.
>>
>>         Thanks,
>>         David
>>
>>
>>                 }
>>
>>
>>             Definitely looks and reads much cleaner.
>>
>>             Dan
>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Thomas Stüfe-2
On Tue, May 30, 2017 at 1:28 PM, David Holmes <[hidden email]>
wrote:

> Correction ...
>
> On 30/05/2017 9:22 PM, David Holmes wrote:
>
>> Hi Thomas,
>>
>> On 30/05/2017 7:53 PM, Thomas Stüfe wrote:
>>
>>> Hi David,
>>>
>>> works fine on AIX, as it did before.
>>>
>>
>> Great - thanks for confirming that again.
>>
>> Had a look at the change, some very small nits:
>>>
>>> - We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
>>> pthread_cond_t). Are the rules not that each header should be
>>> self-contained? If yes, should os_posix.hpp not include the relevant OS
>>> headers for the pthread_... types?
>>>
>>
>> Yes. I'll fix that - and check how the pthread types are currently
>> getting seen. Thanks.
>>
>
> So ... os_posix.hpp only #includes "runtime/os.hpp" and yet uses dozens of
> types defined in numerous other header files! So I could add pthread.h, but
> it would look very lonely.
>
>
Looking more closely, this seems not to be a real header, similar to all
the other os_xxx.hpp files, but its only purpose seems to be to get
included by os.hpp at that one specific place. So, the rules to be
self-contained does not apply, right?

Which also means the include of runtime/os.hpp is a bit pointless.

..Thomas


> Dan: thoughts?
>
> Cheers,
> David
>
>
> - the coding around dlopen/dlsym would be a bit more readable if you were
>>> to define types for the function pointers, that would save you from
>>> spelling them out each time.
>>>
>>
>> I'll safe this for the next cleanup if we share all of the "clock"
>> related stuff.
>>
>> Thanks again,
>> David
>> -----
>>
>> e.g.:
>>>
>>> typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
>>> ...
>>> static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
>>> dlsym(...);
>>>
>>> ...Thomas
>>>
>>>
>>> On Mon, May 29, 2017 at 6:19 AM, David Holmes <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>     Dan, Robbin, Thomas,
>>>
>>>     Okay here is the final ready to push version:
>>>
>>>     http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>>>     <http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/>
>>>
>>>     this fixes all Dan's nits and refactors the time calculation code as
>>>     suggested by Robbin.
>>>
>>>     Thomas: if you are around and able, it would be good to get a final
>>>     sanity check on AIX. Thanks.
>>>
>>>     Testing:
>>>       - JPRT: -testset hotspot
>>>               -testset core
>>>
>>>     - manual:
>>>         - jtreg:java/util/concurrent
>>>         - various little test programs that try to validate sleep/wait
>>>     times to show early returns or unexpected delays
>>>
>>>     Thanks again for the reviews.
>>>
>>>     David
>>>
>>>
>>>     On 29/05/2017 10:29 AM, David Holmes wrote:
>>>
>>>         On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>>
>>>             On 5/26/17 1:27 AM, David Holmes wrote:
>>>
>>>                 Robbin, Dan,
>>>
>>>                 Below is a modified version of the refactored to_abstime
>>>                 code that Robbin suggested.
>>>
>>>                 Robbin: there were a couple of issues with your version.
>>>                 For relative time the timeout is always in nanoseconds -
>>>                 the "unit" only tells you what form the "now_part_sec"
>>>                 is - nanos or micros. And the calc_abs_time always has a
>>>                 deadline in millis. So I simplified and did a little
>>>                 renaming, and tracked max_secs in debug_only instead of
>>>                 returning it.
>>>
>>>                 Please let me know what you think.
>>>
>>>
>>>             Looks OK to me. Nit comments below...
>>>
>>>
>>>         Thanks Dan - more below.
>>>
>>>
>>>
>>>                 // Calculate a new absolute time that is "timeout"
>>>                 nanoseconds from "now".
>>>                 // "unit" indicates the unit of "now_part_sec" (may be
>>>                 nanos or micros depending
>>>                 // on which clock is being used).
>>>                 static void calc_rel_time(timespec* abstime, jlong
>>>                 timeout, jlong now_sec,
>>>                                            jlong now_part_sec, jlong
>>> unit) {
>>>                    time_t max_secs = now_sec + MAX_SECS;
>>>
>>>                    jlong seconds = timeout / NANOUNITS;
>>>                    timeout %= NANOUNITS; // remaining nanos
>>>
>>>                    if (seconds >= MAX_SECS) {
>>>                      // More seconds than we can add, so pin to max_secs.
>>>                      abstime->tv_sec = max_secs;
>>>                      abstime->tv_nsec = 0;
>>>                    } else {
>>>                      abstime->tv_sec = now_sec  + seconds;
>>>                      long nanos = (now_part_sec * (NANOUNITS / unit)) +
>>>                 timeout;
>>>                      if (nanos >= NANOUNITS) { // overflow
>>>                        abstime->tv_sec += 1;
>>>                        nanos -= NANOUNITS;
>>>                      }
>>>                      abstime->tv_nsec = nanos;
>>>                    }
>>>                 }
>>>
>>>                 // Unpack the given deadline in milliseconds since the
>>>                 epoch, into the given timespec.
>>>                 // The current time in seconds is also passed in to
>>>                 enforce an upper bound as discussed above.
>>>                 static void unpack_abs_time(timespec* abstime, jlong
>>>                 deadline, jlong now_sec) {
>>>                    time_t max_secs = now_sec + MAX_SECS;
>>>
>>>                    jlong seconds = deadline / MILLIUNITS;
>>>                    jlong millis = deadline % MILLIUNITS;
>>>
>>>                    if (seconds >= max_secs) {
>>>                      // Absolute seconds exceeds allowed max, so pin to
>>>                 max_secs.
>>>                      abstime->tv_sec = max_secs;
>>>                      abstime->tv_nsec = 0;
>>>                    } else {
>>>                      abstime->tv_sec = seconds;
>>>                      abstime->tv_nsec = millis * (NANOUNITS /
>>> MILLIUNITS);
>>>                    }
>>>                 }
>>>
>>>
>>>                 static void to_abstime(timespec* abstime, jlong timeout,
>>>                 bool isAbsolute) {
>>>
>>>
>>>             There's an extra blank line here.
>>>
>>>
>>>         Fixed.
>>>
>>>
>>>                    DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>
>>>                    if (timeout < 0) {
>>>                      timeout = 0;
>>>                    }
>>>
>>>                 #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>
>>>                    if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>                      struct timespec now;
>>>                      int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>                      assert_status(status == 0, status, "clock_gettime");
>>>                      calc_rel_time(abstime, timeout, now.tv_sec,
>>>                 now.tv_nsec, NANOUNITS);
>>>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>>>                    } else {
>>>
>>>                 #else
>>>
>>>                    { // Match the block scope.
>>>
>>>                 #endif // SUPPORTS_CLOCK_MONOTONIC
>>>
>>>                      // Time-of-day clock is all we can reliably use.
>>>                      struct timeval now;
>>>                      int status = gettimeofday(&now, NULL);
>>>                      assert(status == 0, "gettimeofday");
>>>
>>>
>>>             assert_status() is used above, but assert() is used here.
>>> Why?
>>>
>>>
>>>         Historical. assert_status was introduced for the pthread* and
>>>         other posix funcs that return the error value rather than
>>>         returning -1 and setting errno. gettimeofday is not one of those
>>>         so still has the old assert. However, as someone pointed out a
>>>         while ago you can use assert_status with these and pass errno as
>>>         the "status". So I did that.
>>>
>>>
>>>                      if (isAbsolute) {
>>>                        unpack_abs_time(abstime, timeout, now.tv_sec);
>>>                      }
>>>                      else {
>>>
>>>
>>>             Inconsistent "else-branch" formatting.
>>>             I believe HotSpot style is "} else {"
>>>
>>>
>>>         Fixed.
>>>
>>>                 calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec,
>>>                 MICROUNITS);
>>>                      }
>>>                      DEBUG_ONLY(max_secs += now.tv_sec;)
>>>                    }
>>>
>>>                    assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>                    assert(abstime->tv_sec <= max_secs, "tv_sec >
>>> max_secs");
>>>                    assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>                    assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec
>>>                  >= nanos_per_sec");
>>>
>>>
>>>             Why does the assert mesg have "nanos_per_sec" instead of
>>>             "NANOSECS_PER_SEC"?
>>>
>>>
>>>         No reason. Actually that should now refer to NANOUNITS. Hmmm I
>>>         can not recall why we have NANOUNITS and NANAOSECS_PER_SEC ...
>>>         possibly an oversight.
>>>
>>>             There's an extra blank line here.
>>>
>>>
>>>         Fixed.
>>>
>>>         Will send out complete updated webrev soon.
>>>
>>>         Thanks,
>>>         David
>>>
>>>
>>>                 }
>>>
>>>
>>>             Definitely looks and reads much cleaner.
>>>
>>>             Dan
>>>
>>>
>>>
Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
On 5/30/17 6:20 AM, Thomas Stüfe wrote:

> On Tue, May 30, 2017 at 1:28 PM, David Holmes <[hidden email]>
> wrote:
>
>> Correction ...
>>
>> On 30/05/2017 9:22 PM, David Holmes wrote:
>>
>>> Hi Thomas,
>>>
>>> On 30/05/2017 7:53 PM, Thomas Stüfe wrote:
>>>
>>>> Hi David,
>>>>
>>>> works fine on AIX, as it did before.
>>>>
>>> Great - thanks for confirming that again.
>>>
>>> Had a look at the change, some very small nits:
>>>> - We now carry pthread_.. types in os_posix.hpp (pthread_mutex_t,
>>>> pthread_cond_t). Are the rules not that each header should be
>>>> self-contained? If yes, should os_posix.hpp not include the relevant OS
>>>> headers for the pthread_... types?
>>>>
>>> Yes. I'll fix that - and check how the pthread types are currently
>>> getting seen. Thanks.
>>>
>> So ... os_posix.hpp only #includes "runtime/os.hpp" and yet uses dozens of
>> types defined in numerous other header files! So I could add pthread.h, but
>> it would look very lonely.
>>
>>
> Looking more closely, this seems not to be a real header, similar to all
> the other os_xxx.hpp files, but its only purpose seems to be to get
> included by os.hpp at that one specific place. So, the rules to be
> self-contained does not apply, right?
>
> Which also means the include of runtime/os.hpp is a bit pointless.
>
> ..Thomas
>
>
>> Dan: thoughts?

Just starting my re-review now, but I'm inclined to agree os_posix.hpp
is not meant to be a self-contained header...

Dan


>>
>> Cheers,
>> David
>>
>>
>> - the coding around dlopen/dlsym would be a bit more readable if you were
>>>> to define types for the function pointers, that would save you from
>>>> spelling them out each time.
>>>>
>>> I'll safe this for the next cleanup if we share all of the "clock"
>>> related stuff.
>>>
>>> Thanks again,
>>> David
>>> -----
>>>
>>> e.g.:
>>>> typedef int (*clock_getres_func_t)(clockid_t, struct timespec *);
>>>> ...
>>>> static clock_getres_func_t _clock_getres_func = (clock_getres_func_t)
>>>> dlsym(...);
>>>>
>>>> ...Thomas
>>>>
>>>>
>>>> On Mon, May 29, 2017 at 6:19 AM, David Holmes <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>      Dan, Robbin, Thomas,
>>>>
>>>>      Okay here is the final ready to push version:
>>>>
>>>>      http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>>>>      <http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/>
>>>>
>>>>      this fixes all Dan's nits and refactors the time calculation code as
>>>>      suggested by Robbin.
>>>>
>>>>      Thomas: if you are around and able, it would be good to get a final
>>>>      sanity check on AIX. Thanks.
>>>>
>>>>      Testing:
>>>>        - JPRT: -testset hotspot
>>>>                -testset core
>>>>
>>>>      - manual:
>>>>          - jtreg:java/util/concurrent
>>>>          - various little test programs that try to validate sleep/wait
>>>>      times to show early returns or unexpected delays
>>>>
>>>>      Thanks again for the reviews.
>>>>
>>>>      David
>>>>
>>>>
>>>>      On 29/05/2017 10:29 AM, David Holmes wrote:
>>>>
>>>>          On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>>>
>>>>              On 5/26/17 1:27 AM, David Holmes wrote:
>>>>
>>>>                  Robbin, Dan,
>>>>
>>>>                  Below is a modified version of the refactored to_abstime
>>>>                  code that Robbin suggested.
>>>>
>>>>                  Robbin: there were a couple of issues with your version.
>>>>                  For relative time the timeout is always in nanoseconds -
>>>>                  the "unit" only tells you what form the "now_part_sec"
>>>>                  is - nanos or micros. And the calc_abs_time always has a
>>>>                  deadline in millis. So I simplified and did a little
>>>>                  renaming, and tracked max_secs in debug_only instead of
>>>>                  returning it.
>>>>
>>>>                  Please let me know what you think.
>>>>
>>>>
>>>>              Looks OK to me. Nit comments below...
>>>>
>>>>
>>>>          Thanks Dan - more below.
>>>>
>>>>
>>>>
>>>>                  // Calculate a new absolute time that is "timeout"
>>>>                  nanoseconds from "now".
>>>>                  // "unit" indicates the unit of "now_part_sec" (may be
>>>>                  nanos or micros depending
>>>>                  // on which clock is being used).
>>>>                  static void calc_rel_time(timespec* abstime, jlong
>>>>                  timeout, jlong now_sec,
>>>>                                             jlong now_part_sec, jlong
>>>> unit) {
>>>>                     time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>                     jlong seconds = timeout / NANOUNITS;
>>>>                     timeout %= NANOUNITS; // remaining nanos
>>>>
>>>>                     if (seconds >= MAX_SECS) {
>>>>                       // More seconds than we can add, so pin to max_secs.
>>>>                       abstime->tv_sec = max_secs;
>>>>                       abstime->tv_nsec = 0;
>>>>                     } else {
>>>>                       abstime->tv_sec = now_sec  + seconds;
>>>>                       long nanos = (now_part_sec * (NANOUNITS / unit)) +
>>>>                  timeout;
>>>>                       if (nanos >= NANOUNITS) { // overflow
>>>>                         abstime->tv_sec += 1;
>>>>                         nanos -= NANOUNITS;
>>>>                       }
>>>>                       abstime->tv_nsec = nanos;
>>>>                     }
>>>>                  }
>>>>
>>>>                  // Unpack the given deadline in milliseconds since the
>>>>                  epoch, into the given timespec.
>>>>                  // The current time in seconds is also passed in to
>>>>                  enforce an upper bound as discussed above.
>>>>                  static void unpack_abs_time(timespec* abstime, jlong
>>>>                  deadline, jlong now_sec) {
>>>>                     time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>                     jlong seconds = deadline / MILLIUNITS;
>>>>                     jlong millis = deadline % MILLIUNITS;
>>>>
>>>>                     if (seconds >= max_secs) {
>>>>                       // Absolute seconds exceeds allowed max, so pin to
>>>>                  max_secs.
>>>>                       abstime->tv_sec = max_secs;
>>>>                       abstime->tv_nsec = 0;
>>>>                     } else {
>>>>                       abstime->tv_sec = seconds;
>>>>                       abstime->tv_nsec = millis * (NANOUNITS /
>>>> MILLIUNITS);
>>>>                     }
>>>>                  }
>>>>
>>>>
>>>>                  static void to_abstime(timespec* abstime, jlong timeout,
>>>>                  bool isAbsolute) {
>>>>
>>>>
>>>>              There's an extra blank line here.
>>>>
>>>>
>>>>          Fixed.
>>>>
>>>>
>>>>                     DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>>
>>>>                     if (timeout < 0) {
>>>>                       timeout = 0;
>>>>                     }
>>>>
>>>>                  #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>                     if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>>                       struct timespec now;
>>>>                       int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>                       assert_status(status == 0, status, "clock_gettime");
>>>>                       calc_rel_time(abstime, timeout, now.tv_sec,
>>>>                  now.tv_nsec, NANOUNITS);
>>>>                       DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>                     } else {
>>>>
>>>>                  #else
>>>>
>>>>                     { // Match the block scope.
>>>>
>>>>                  #endif // SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>                       // Time-of-day clock is all we can reliably use.
>>>>                       struct timeval now;
>>>>                       int status = gettimeofday(&now, NULL);
>>>>                       assert(status == 0, "gettimeofday");
>>>>
>>>>
>>>>              assert_status() is used above, but assert() is used here.
>>>> Why?
>>>>
>>>>
>>>>          Historical. assert_status was introduced for the pthread* and
>>>>          other posix funcs that return the error value rather than
>>>>          returning -1 and setting errno. gettimeofday is not one of those
>>>>          so still has the old assert. However, as someone pointed out a
>>>>          while ago you can use assert_status with these and pass errno as
>>>>          the "status". So I did that.
>>>>
>>>>
>>>>                       if (isAbsolute) {
>>>>                         unpack_abs_time(abstime, timeout, now.tv_sec);
>>>>                       }
>>>>                       else {
>>>>
>>>>
>>>>              Inconsistent "else-branch" formatting.
>>>>              I believe HotSpot style is "} else {"
>>>>
>>>>
>>>>          Fixed.
>>>>
>>>>                  calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec,
>>>>                  MICROUNITS);
>>>>                       }
>>>>                       DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>                     }
>>>>
>>>>                     assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>>                     assert(abstime->tv_sec <= max_secs, "tv_sec >
>>>> max_secs");
>>>>                     assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>>                     assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec
>>>>                   >= nanos_per_sec");
>>>>
>>>>
>>>>              Why does the assert mesg have "nanos_per_sec" instead of
>>>>              "NANOSECS_PER_SEC"?
>>>>
>>>>
>>>>          No reason. Actually that should now refer to NANOUNITS. Hmmm I
>>>>          can not recall why we have NANOUNITS and NANAOSECS_PER_SEC ...
>>>>          possibly an oversight.
>>>>
>>>>              There's an extra blank line here.
>>>>
>>>>
>>>>          Fixed.
>>>>
>>>>          Will send out complete updated webrev soon.
>>>>
>>>>          Thanks,
>>>>          David
>>>>
>>>>
>>>>                  }
>>>>
>>>>
>>>>              Definitely looks and reads much cleaner.
>>>>
>>>>              Dan
>>>>
>>>>
>>>>

Reply | Threaded
Open this post in threaded view
|

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

Daniel D. Daugherty
In reply to this post by David Holmes
On 5/28/17 10:19 PM, David Holmes wrote:
> Dan, Robbin, Thomas,
>
> Okay here is the final ready to push version:
>
> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/

General
   - Approaching the review differently than last round. This time I'm
     focused on the os_posix.[ch]pp changes as if this were all new code.
   - i.e., I'm going to assume that code deleted from the platform
     specific files is all appropriately represented in os_posix.[ch]pp.

src/os/posix/vm/os_posix.hpp
     No comments.

src/os/posix/vm/os_posix.cpp
     L1518:     _use_clock_monotonic_condattr = true;
     L1522:         _use_clock_monotonic_condattr = false;
         _use_clock_monotonic_condattr could briefly be observed as 'true'
         before being reset to 'false' due to the EINVAL. I think we are
         single threaded at this point so there should be no other thread
         running to be confused by this.

         An alternative would be to set _use_clock_monotonic_condattr
         to true only when _pthread_condattr_setclock() returns 0.

     L1581: // number of seconds, in abstime, is less than current_time
+ 100,000,000.
     L1582: // As it will be over 20 years before "now + 100000000" will
overflow we can
     L1584: // of "now + 100,000,000". This places a limit on the
timeout of about 3.17
         nit - consistency of using ',' or not in 100000000. Personally,
             I would prefer no commas so the comments match MAX_SECS.

     L1703:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
     L1743:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
         nit - please add spaces around the '-' operator.

     L1749:     to_abstime(&abst, millis * (NANOUNITS/MILLIUNITS), false);
         nit - please add spaces around the '/' operator.

src/os/aix/vm/os_aix.hpp
     No comments.

src/os/aix/vm/os_aix.cpp
     No comments.

src/os/bsd/vm/os_bsd.hpp
     No comments.

src/os/bsd/vm/os_bsd.cpp
     No comments.

src/os/linux/vm/os_linux.hpp
     No comments.

src/os/linux/vm/os_linux.cpp
     No comments.

src/os/solaris/vm/os_solaris.hpp
     No comments.

src/os/solaris/vm/os_solaris.cpp
     No comments.


Thumbs up. Don't need to see another webrev if you choose to fix
the bits...

Dan


>
> this fixes all Dan's nits and refactors the time calculation code as
> suggested by Robbin.
>
> Thomas: if you are around and able, it would be good to get a final
> sanity check on AIX. Thanks.
>
> Testing:
>  - JPRT: -testset hotspot
>          -testset core
>
> - manual:
>    - jtreg:java/util/concurrent
>    - various little test programs that try to validate sleep/wait
> times to show early returns or unexpected delays
>
> Thanks again for the reviews.
>
> David
>
> On 29/05/2017 10:29 AM, David Holmes wrote:
>> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>> On 5/26/17 1:27 AM, David Holmes wrote:
>>>> Robbin, Dan,
>>>>
>>>> Below is a modified version of the refactored to_abstime code that
>>>> Robbin suggested.
>>>>
>>>> Robbin: there were a couple of issues with your version. For
>>>> relative time the timeout is always in nanoseconds - the "unit"
>>>> only tells you what form the "now_part_sec" is - nanos or micros.
>>>> And the calc_abs_time always has a deadline in millis. So I
>>>> simplified and did a little renaming, and tracked max_secs in
>>>> debug_only instead of returning it.
>>>>
>>>> Please let me know what you think.
>>>
>>> Looks OK to me. Nit comments below...
>>
>> Thanks Dan - more below.
>>
>>>>
>>>>
>>>> // Calculate a new absolute time that is "timeout" nanoseconds from
>>>> "now".
>>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or
>>>> micros depending
>>>> // on which clock is being used).
>>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>>>> now_sec,
>>>>                           jlong now_part_sec, jlong unit) {
>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>   jlong seconds = timeout / NANOUNITS;
>>>>   timeout %= NANOUNITS; // remaining nanos
>>>>
>>>>   if (seconds >= MAX_SECS) {
>>>>     // More seconds than we can add, so pin to max_secs.
>>>>     abstime->tv_sec = max_secs;
>>>>     abstime->tv_nsec = 0;
>>>>   } else {
>>>>     abstime->tv_sec = now_sec  + seconds;
>>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>>     if (nanos >= NANOUNITS) { // overflow
>>>>       abstime->tv_sec += 1;
>>>>       nanos -= NANOUNITS;
>>>>     }
>>>>     abstime->tv_nsec = nanos;
>>>>   }
>>>> }
>>>>
>>>> // Unpack the given deadline in milliseconds since the epoch, into
>>>> the given timespec.
>>>> // The current time in seconds is also passed in to enforce an
>>>> upper bound as discussed above.
>>>> static void unpack_abs_time(timespec* abstime, jlong deadline,
>>>> jlong now_sec) {
>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>
>>>>   jlong seconds = deadline / MILLIUNITS;
>>>>   jlong millis = deadline % MILLIUNITS;
>>>>
>>>>   if (seconds >= max_secs) {
>>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>>     abstime->tv_sec = max_secs;
>>>>     abstime->tv_nsec = 0;
>>>>   } else {
>>>>     abstime->tv_sec = seconds;
>>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>>   }
>>>> }
>>>>
>>>>
>>>> static void to_abstime(timespec* abstime, jlong timeout, bool
>>>> isAbsolute) {
>>>
>>> There's an extra blank line here.
>>
>> Fixed.
>>
>>>>
>>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>>
>>>>   if (timeout < 0) {
>>>>     timeout = 0;
>>>>   }
>>>>
>>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>>     struct timespec now;
>>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>     assert_status(status == 0, status, "clock_gettime");
>>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec,
>>>> NANOUNITS);
>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>   } else {
>>>>
>>>> #else
>>>>
>>>>   { // Match the block scope.
>>>>
>>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>>
>>>>     // Time-of-day clock is all we can reliably use.
>>>>     struct timeval now;
>>>>     int status = gettimeofday(&now, NULL);
>>>>     assert(status == 0, "gettimeofday");
>>>
>>> assert_status() is used above, but assert() is used here. Why?
>>
>> Historical. assert_status was introduced for the pthread* and other
>> posix funcs that return the error value rather than returning -1 and
>> setting errno. gettimeofday is not one of those so still has the old
>> assert. However, as someone pointed out a while ago you can use
>> assert_status with these and pass errno as the "status". So I did that.
>>
>>>
>>>>     if (isAbsolute) {
>>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>>     }
>>>>     else {
>>>
>>> Inconsistent "else-branch" formatting.
>>> I believe HotSpot style is "} else {"
>>
>> Fixed.
>>
>>>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>     }
>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>   }
>>>>
>>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>>>> nanos_per_sec");
>>>
>>> Why does the assert mesg have "nanos_per_sec" instead of
>>> "NANOSECS_PER_SEC"?
>>
>> No reason. Actually that should now refer to NANOUNITS. Hmmm I can
>> not recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly
>> an oversight.
>>
>>> There's an extra blank line here.
>>
>> Fixed.
>>
>> Will send out complete updated webrev soon.
>>
>> Thanks,
>> David
>>
>>>>
>>>> }
>>>
>>> Definitely looks and reads much cleaner.
>>>
>>> Dan
>>>

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
Hi Dan,

On 31/05/2017 1:11 AM, Daniel D. Daugherty wrote:

> On 5/28/17 10:19 PM, David Holmes wrote:
>> Dan, Robbin, Thomas,
>>
>> Okay here is the final ready to push version:
>>
>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>
> General
>    - Approaching the review differently than last round. This time I'm
>      focused on the os_posix.[ch]pp changes as if this were all new code.
>    - i.e., I'm going to assume that code deleted from the platform
>      specific files is all appropriately represented in os_posix.[ch]pp.

Okay - thanks again.

> src/os/posix/vm/os_posix.hpp
>      No comments.

Okay I'm leaving the #includes as-is.

> src/os/posix/vm/os_posix.cpp
>      L1518:     _use_clock_monotonic_condattr = true;
>      L1522:         _use_clock_monotonic_condattr = false;
>          _use_clock_monotonic_condattr could briefly be observed as 'true'
>          before being reset to 'false' due to the EINVAL. I think we are
>          single threaded at this point so there should be no other thread
>          running to be confused by this.

Right this is single-threaded VM init.

>          An alternative would be to set _use_clock_monotonic_condattr
>          to true only when _pthread_condattr_setclock() returns 0.

Yes - fixed.

>      L1581: // number of seconds, in abstime, is less than current_time
> + 100,000,000.
>      L1582: // As it will be over 20 years before "now + 100000000" will
> overflow we can
>      L1584: // of "now + 100,000,000". This places a limit on the
> timeout of about 3.17
>          nit - consistency of using ',' or not in 100000000. Personally,
>              I would prefer no commas so the comments match MAX_SECS.

Fixed.

>      L1703:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
>      L1743:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
>          nit - please add spaces around the '-' operator.

Fixed.

>      L1749:     to_abstime(&abst, millis * (NANOUNITS/MILLIUNITS), false);
>          nit - please add spaces around the '/' operator.

Fixed.

> src/os/aix/vm/os_aix.hpp
>      No comments.
>
> src/os/aix/vm/os_aix.cpp
>      No comments.
>
> src/os/bsd/vm/os_bsd.hpp
>      No comments.
>
> src/os/bsd/vm/os_bsd.cpp
>      No comments.
>
> src/os/linux/vm/os_linux.hpp
>      No comments.
>
> src/os/linux/vm/os_linux.cpp
>      No comments.
>
> src/os/solaris/vm/os_solaris.hpp
>      No comments.
>
> src/os/solaris/vm/os_solaris.cpp
>      No comments.
>
>
> Thumbs up. Don't need to see another webrev if you choose to fix
> the bits...

Thanks again.

David

> Dan
>
>
>>
>> this fixes all Dan's nits and refactors the time calculation code as
>> suggested by Robbin.
>>
>> Thomas: if you are around and able, it would be good to get a final
>> sanity check on AIX. Thanks.
>>
>> Testing:
>>  - JPRT: -testset hotspot
>>          -testset core
>>
>> - manual:
>>    - jtreg:java/util/concurrent
>>    - various little test programs that try to validate sleep/wait
>> times to show early returns or unexpected delays
>>
>> Thanks again for the reviews.
>>
>> David
>>
>> On 29/05/2017 10:29 AM, David Holmes wrote:
>>> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>>> On 5/26/17 1:27 AM, David Holmes wrote:
>>>>> Robbin, Dan,
>>>>>
>>>>> Below is a modified version of the refactored to_abstime code that
>>>>> Robbin suggested.
>>>>>
>>>>> Robbin: there were a couple of issues with your version. For
>>>>> relative time the timeout is always in nanoseconds - the "unit"
>>>>> only tells you what form the "now_part_sec" is - nanos or micros.
>>>>> And the calc_abs_time always has a deadline in millis. So I
>>>>> simplified and did a little renaming, and tracked max_secs in
>>>>> debug_only instead of returning it.
>>>>>
>>>>> Please let me know what you think.
>>>>
>>>> Looks OK to me. Nit comments below...
>>>
>>> Thanks Dan - more below.
>>>
>>>>>
>>>>>
>>>>> // Calculate a new absolute time that is "timeout" nanoseconds from
>>>>> "now".
>>>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or
>>>>> micros depending
>>>>> // on which clock is being used).
>>>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>>>>> now_sec,
>>>>>                           jlong now_part_sec, jlong unit) {
>>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>>
>>>>>   jlong seconds = timeout / NANOUNITS;
>>>>>   timeout %= NANOUNITS; // remaining nanos
>>>>>
>>>>>   if (seconds >= MAX_SECS) {
>>>>>     // More seconds than we can add, so pin to max_secs.
>>>>>     abstime->tv_sec = max_secs;
>>>>>     abstime->tv_nsec = 0;
>>>>>   } else {
>>>>>     abstime->tv_sec = now_sec  + seconds;
>>>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>>>     if (nanos >= NANOUNITS) { // overflow
>>>>>       abstime->tv_sec += 1;
>>>>>       nanos -= NANOUNITS;
>>>>>     }
>>>>>     abstime->tv_nsec = nanos;
>>>>>   }
>>>>> }
>>>>>
>>>>> // Unpack the given deadline in milliseconds since the epoch, into
>>>>> the given timespec.
>>>>> // The current time in seconds is also passed in to enforce an
>>>>> upper bound as discussed above.
>>>>> static void unpack_abs_time(timespec* abstime, jlong deadline,
>>>>> jlong now_sec) {
>>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>>
>>>>>   jlong seconds = deadline / MILLIUNITS;
>>>>>   jlong millis = deadline % MILLIUNITS;
>>>>>
>>>>>   if (seconds >= max_secs) {
>>>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>>>     abstime->tv_sec = max_secs;
>>>>>     abstime->tv_nsec = 0;
>>>>>   } else {
>>>>>     abstime->tv_sec = seconds;
>>>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>>>   }
>>>>> }
>>>>>
>>>>>
>>>>> static void to_abstime(timespec* abstime, jlong timeout, bool
>>>>> isAbsolute) {
>>>>
>>>> There's an extra blank line here.
>>>
>>> Fixed.
>>>
>>>>>
>>>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>>>
>>>>>   if (timeout < 0) {
>>>>>     timeout = 0;
>>>>>   }
>>>>>
>>>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>>
>>>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>>>     struct timespec now;
>>>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>     assert_status(status == 0, status, "clock_gettime");
>>>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec,
>>>>> NANOUNITS);
>>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>>   } else {
>>>>>
>>>>> #else
>>>>>
>>>>>   { // Match the block scope.
>>>>>
>>>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>>>
>>>>>     // Time-of-day clock is all we can reliably use.
>>>>>     struct timeval now;
>>>>>     int status = gettimeofday(&now, NULL);
>>>>>     assert(status == 0, "gettimeofday");
>>>>
>>>> assert_status() is used above, but assert() is used here. Why?
>>>
>>> Historical. assert_status was introduced for the pthread* and other
>>> posix funcs that return the error value rather than returning -1 and
>>> setting errno. gettimeofday is not one of those so still has the old
>>> assert. However, as someone pointed out a while ago you can use
>>> assert_status with these and pass errno as the "status". So I did that.
>>>
>>>>
>>>>>     if (isAbsolute) {
>>>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>>>     }
>>>>>     else {
>>>>
>>>> Inconsistent "else-branch" formatting.
>>>> I believe HotSpot style is "} else {"
>>>
>>> Fixed.
>>>
>>>>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>>     }
>>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>>   }
>>>>>
>>>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>>>>> nanos_per_sec");
>>>>
>>>> Why does the assert mesg have "nanos_per_sec" instead of
>>>> "NANOSECS_PER_SEC"?
>>>
>>> No reason. Actually that should now refer to NANOUNITS. Hmmm I can
>>> not recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly
>>> an oversight.
>>>
>>>> There's an extra blank line here.
>>>
>>> Fixed.
>>>
>>> Will send out complete updated webrev soon.
>>>
>>> Thanks,
>>> David
>>>
>>>>>
>>>>> }
>>>>
>>>> Definitely looks and reads much cleaner.
>>>>
>>>> Dan
>>>>
>
Reply | Threaded
Open this post in threaded view
|

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

David Holmes
As is typical my change caused a breakage on newer Mac OS (Sierra)
systems that we don't have in JPRT:

https://bugs.openjdk.java.net/browse/JDK-8181451

Ironically the proposed fix is the same typedef cleanup that Thomas had
suggested earlier, but which I didn't take up. <sigh>

I'm trying to find a system I can actually investigate this on. IIUC
these newer Mac's do have clock_gettime (others don't!), but we don't
know how well it works.

David

On 31/05/2017 6:50 AM, David Holmes wrote:

> Hi Dan,
>
> On 31/05/2017 1:11 AM, Daniel D. Daugherty wrote:
>> On 5/28/17 10:19 PM, David Holmes wrote:
>>> Dan, Robbin, Thomas,
>>>
>>> Okay here is the final ready to push version:
>>>
>>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot.v2/
>>
>> General
>>    - Approaching the review differently than last round. This time I'm
>>      focused on the os_posix.[ch]pp changes as if this were all new code.
>>    - i.e., I'm going to assume that code deleted from the platform
>>      specific files is all appropriately represented in os_posix.[ch]pp.
>
> Okay - thanks again.
>
>> src/os/posix/vm/os_posix.hpp
>>      No comments.
>
> Okay I'm leaving the #includes as-is.
>
>> src/os/posix/vm/os_posix.cpp
>>      L1518:     _use_clock_monotonic_condattr = true;
>>      L1522:         _use_clock_monotonic_condattr = false;
>>          _use_clock_monotonic_condattr could briefly be observed as
>> 'true'
>>          before being reset to 'false' due to the EINVAL. I think we are
>>          single threaded at this point so there should be no other thread
>>          running to be confused by this.
>
> Right this is single-threaded VM init.
>
>>          An alternative would be to set _use_clock_monotonic_condattr
>>          to true only when _pthread_condattr_setclock() returns 0.
>
> Yes - fixed.
>
>>      L1581: // number of seconds, in abstime, is less than
>> current_time + 100,000,000.
>>      L1582: // As it will be over 20 years before "now + 100000000"
>> will overflow we can
>>      L1584: // of "now + 100,000,000". This places a limit on the
>> timeout of about 3.17
>>          nit - consistency of using ',' or not in 100000000. Personally,
>>              I would prefer no commas so the comments match MAX_SECS.
>
> Fixed.
>
>>      L1703:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
>>      L1743:     if (Atomic::cmpxchg(v-1, &_event, v) == v) break;
>>          nit - please add spaces around the '-' operator.
>
> Fixed.
>
>>      L1749:     to_abstime(&abst, millis * (NANOUNITS/MILLIUNITS),
>> false);
>>          nit - please add spaces around the '/' operator.
>
> Fixed.
>
>> src/os/aix/vm/os_aix.hpp
>>      No comments.
>>
>> src/os/aix/vm/os_aix.cpp
>>      No comments.
>>
>> src/os/bsd/vm/os_bsd.hpp
>>      No comments.
>>
>> src/os/bsd/vm/os_bsd.cpp
>>      No comments.
>>
>> src/os/linux/vm/os_linux.hpp
>>      No comments.
>>
>> src/os/linux/vm/os_linux.cpp
>>      No comments.
>>
>> src/os/solaris/vm/os_solaris.hpp
>>      No comments.
>>
>> src/os/solaris/vm/os_solaris.cpp
>>      No comments.
>>
>>
>> Thumbs up. Don't need to see another webrev if you choose to fix
>> the bits...
>
> Thanks again.
>
> David
>
>> Dan
>>
>>
>>>
>>> this fixes all Dan's nits and refactors the time calculation code as
>>> suggested by Robbin.
>>>
>>> Thomas: if you are around and able, it would be good to get a final
>>> sanity check on AIX. Thanks.
>>>
>>> Testing:
>>>  - JPRT: -testset hotspot
>>>          -testset core
>>>
>>> - manual:
>>>    - jtreg:java/util/concurrent
>>>    - various little test programs that try to validate sleep/wait
>>> times to show early returns or unexpected delays
>>>
>>> Thanks again for the reviews.
>>>
>>> David
>>>
>>> On 29/05/2017 10:29 AM, David Holmes wrote:
>>>> On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote:
>>>>> On 5/26/17 1:27 AM, David Holmes wrote:
>>>>>> Robbin, Dan,
>>>>>>
>>>>>> Below is a modified version of the refactored to_abstime code that
>>>>>> Robbin suggested.
>>>>>>
>>>>>> Robbin: there were a couple of issues with your version. For
>>>>>> relative time the timeout is always in nanoseconds - the "unit"
>>>>>> only tells you what form the "now_part_sec" is - nanos or micros.
>>>>>> And the calc_abs_time always has a deadline in millis. So I
>>>>>> simplified and did a little renaming, and tracked max_secs in
>>>>>> debug_only instead of returning it.
>>>>>>
>>>>>> Please let me know what you think.
>>>>>
>>>>> Looks OK to me. Nit comments below...
>>>>
>>>> Thanks Dan - more below.
>>>>
>>>>>>
>>>>>>
>>>>>> // Calculate a new absolute time that is "timeout" nanoseconds
>>>>>> from "now".
>>>>>> // "unit" indicates the unit of "now_part_sec" (may be nanos or
>>>>>> micros depending
>>>>>> // on which clock is being used).
>>>>>> static void calc_rel_time(timespec* abstime, jlong timeout, jlong
>>>>>> now_sec,
>>>>>>                           jlong now_part_sec, jlong unit) {
>>>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>>>
>>>>>>   jlong seconds = timeout / NANOUNITS;
>>>>>>   timeout %= NANOUNITS; // remaining nanos
>>>>>>
>>>>>>   if (seconds >= MAX_SECS) {
>>>>>>     // More seconds than we can add, so pin to max_secs.
>>>>>>     abstime->tv_sec = max_secs;
>>>>>>     abstime->tv_nsec = 0;
>>>>>>   } else {
>>>>>>     abstime->tv_sec = now_sec  + seconds;
>>>>>>     long nanos = (now_part_sec * (NANOUNITS / unit)) + timeout;
>>>>>>     if (nanos >= NANOUNITS) { // overflow
>>>>>>       abstime->tv_sec += 1;
>>>>>>       nanos -= NANOUNITS;
>>>>>>     }
>>>>>>     abstime->tv_nsec = nanos;
>>>>>>   }
>>>>>> }
>>>>>>
>>>>>> // Unpack the given deadline in milliseconds since the epoch, into
>>>>>> the given timespec.
>>>>>> // The current time in seconds is also passed in to enforce an
>>>>>> upper bound as discussed above.
>>>>>> static void unpack_abs_time(timespec* abstime, jlong deadline,
>>>>>> jlong now_sec) {
>>>>>>   time_t max_secs = now_sec + MAX_SECS;
>>>>>>
>>>>>>   jlong seconds = deadline / MILLIUNITS;
>>>>>>   jlong millis = deadline % MILLIUNITS;
>>>>>>
>>>>>>   if (seconds >= max_secs) {
>>>>>>     // Absolute seconds exceeds allowed max, so pin to max_secs.
>>>>>>     abstime->tv_sec = max_secs;
>>>>>>     abstime->tv_nsec = 0;
>>>>>>   } else {
>>>>>>     abstime->tv_sec = seconds;
>>>>>>     abstime->tv_nsec = millis * (NANOUNITS / MILLIUNITS);
>>>>>>   }
>>>>>> }
>>>>>>
>>>>>>
>>>>>> static void to_abstime(timespec* abstime, jlong timeout, bool
>>>>>> isAbsolute) {
>>>>>
>>>>> There's an extra blank line here.
>>>>
>>>> Fixed.
>>>>
>>>>>>
>>>>>>   DEBUG_ONLY(int max_secs = MAX_SECS;)
>>>>>>
>>>>>>   if (timeout < 0) {
>>>>>>     timeout = 0;
>>>>>>   }
>>>>>>
>>>>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>>>
>>>>>>   if (_use_clock_monotonic_condattr && !isAbsolute) {
>>>>>>     struct timespec now;
>>>>>>     int status = _clock_gettime(CLOCK_MONOTONIC, &now);
>>>>>>     assert_status(status == 0, status, "clock_gettime");
>>>>>>     calc_rel_time(abstime, timeout, now.tv_sec, now.tv_nsec,
>>>>>> NANOUNITS);
>>>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>>>   } else {
>>>>>>
>>>>>> #else
>>>>>>
>>>>>>   { // Match the block scope.
>>>>>>
>>>>>> #endif // SUPPORTS_CLOCK_MONOTONIC
>>>>>>
>>>>>>     // Time-of-day clock is all we can reliably use.
>>>>>>     struct timeval now;
>>>>>>     int status = gettimeofday(&now, NULL);
>>>>>>     assert(status == 0, "gettimeofday");
>>>>>
>>>>> assert_status() is used above, but assert() is used here. Why?
>>>>
>>>> Historical. assert_status was introduced for the pthread* and other
>>>> posix funcs that return the error value rather than returning -1 and
>>>> setting errno. gettimeofday is not one of those so still has the old
>>>> assert. However, as someone pointed out a while ago you can use
>>>> assert_status with these and pass errno as the "status". So I did that.
>>>>
>>>>>
>>>>>>     if (isAbsolute) {
>>>>>>       unpack_abs_time(abstime, timeout, now.tv_sec);
>>>>>>     }
>>>>>>     else {
>>>>>
>>>>> Inconsistent "else-branch" formatting.
>>>>> I believe HotSpot style is "} else {"
>>>>
>>>> Fixed.
>>>>
>>>>>> calc_rel_time(abstime, timeout, now.tv_sec, now.tv_usec, MICROUNITS);
>>>>>>     }
>>>>>>     DEBUG_ONLY(max_secs += now.tv_sec;)
>>>>>>   }
>>>>>>
>>>>>>   assert(abstime->tv_sec >= 0, "tv_sec < 0");
>>>>>>   assert(abstime->tv_sec <= max_secs, "tv_sec > max_secs");
>>>>>>   assert(abstime->tv_nsec >= 0, "tv_nsec < 0");
>>>>>>   assert(abstime->tv_nsec < NANOSECS_PER_SEC, "tv_nsec >=
>>>>>> nanos_per_sec");
>>>>>
>>>>> Why does the assert mesg have "nanos_per_sec" instead of
>>>>> "NANOSECS_PER_SEC"?
>>>>
>>>> No reason. Actually that should now refer to NANOUNITS. Hmmm I can
>>>> not recall why we have NANOUNITS and NANAOSECS_PER_SEC ... possibly
>>>> an oversight.
>>>>
>>>>> There's an extra blank line here.
>>>>
>>>> Fixed.
>>>>
>>>> Will send out complete updated webrev soon.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>>
>>>>>> }
>>>>>
>>>>> Definitely looks and reads much cleaner.
>>>>>
>>>>> Dan
>>>>>
>>