Quantcast

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

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

David Holmes
Bug: https://bugs.openjdk.java.net/browse/JDK-8174231

webrevs:

Build-related: http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
       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
|  
Report Content as Inappropriate

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

Magnus Ihse Bursie
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.

/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
|  
Report Content as Inappropriate

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

David Holmes
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.

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
|  
Report Content as Inappropriate

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

Magnus Ihse Bursie


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.

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
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Hi David, Magnus,

compiles and works fine on AIX, but as mentioned before off-list to David I
see this stdout:

configure: No CLOCK_GETTIME_IN_LIBRT
configure: No CLOCK_GETTIME_IN_LIBRT

Also, the -DSUPPORTS_CLOCK_MONOTONIC appears twice on the command line.
Full compile command looks like this:

/bin/xlC_r -q64 -qpic -D_REENTRANT -D__STDC_FORMAT_MACROS
-DSUPPORTS_CLOCK_MONOTONIC -DSUPPORTS_CLOCK_MONOTONIC -DAIX -qtune=balanced
-qalias=noansi -qstrict -qtls=default -qlanglvl=c99vla -qlanglvl=noredefmac
-qnortti -qnoeh -qignerrno -qarch=ppc64 -DASSERT -DTARGET_ARCH_ppc
-DINCLUDE_SUFFIX_OS=_aix -DINCLUDE_SUFFIX_CPU=_ppc -DTARGET_COMPILER_xlc
-DPPC64 -DHOTSPOT_LIB_ARCH='"ppc64"' -DCOMPILER1 -DCOMPILER2
-DINCLUDE_JVMCI=0 -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/aix/vm
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/cpu/ppc/vm
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os_cpu/aix_ppc/vm
-I/priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/gensrc
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/precompiled
-I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/prims
-DDONT_USE_PRECOMPILED_HEADER -g -qsuppress=1540-0216 -qsuppress=1540-0198
-qsuppress=1540-1090 -qsuppress=1540-1639 -qsuppress=1540-1088
-qsuppress=1500-010 -O3 -qhot=level=1 -qinline -qinlglue
-DTHIS_FILE='"os_posix.cpp"' -c -qmakedep=gcc -MF
/priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.d
-o /priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.o
/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm/os_posix.cpp

 -DSUPPORTS_CLOCK_MONOTONIC is the only switch appearing twice. I'm
baffled. Do you have any idea?

Regards, Thomas


On Thu, May 18, 2017 at 12:06 PM, Magnus Ihse Bursie <
[hidden email]> 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.
>
> 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
|  
Report Content as Inappropriate

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

Thomas Stüfe-2
Okay, I regenerated the generated-configure.sh and the double definitions
for -DSUPPORTS_CLOCK_MONOTONIC disappeared. So the generated-configure.sh
you posted was just outdated.

However, the debug output still appears twice: configure: No
CLOCK_GETTIME_IN_LIBRT

AC_MSG_NOTICE([No CLOCK_GETTIME_IN_LIBRT]) expands to two invocations of
ac_echo(..). I am out of my depth here, not sure what the background is.
But as you want to remove the debug output anyway, I think this is not an
issue.

I will take more time for a full review later. Just adding that I really
like this fix, it takes a lot of coding off our (platform specific) backs,
which is a good thing!

Kind Regards, Thomas

On Thu, May 18, 2017 at 4:40 PM, Thomas Stüfe <[hidden email]>
wrote:

> Hi David, Magnus,
>
> compiles and works fine on AIX, but as mentioned before off-list to David
> I see this stdout:
>
> configure: No CLOCK_GETTIME_IN_LIBRT
> configure: No CLOCK_GETTIME_IN_LIBRT
>
> Also, the -DSUPPORTS_CLOCK_MONOTONIC appears twice on the command line.
> Full compile command looks like this:
>
> /bin/xlC_r -q64 -qpic -D_REENTRANT -D__STDC_FORMAT_MACROS
> -DSUPPORTS_CLOCK_MONOTONIC -DSUPPORTS_CLOCK_MONOTONIC -DAIX -qtune=balanced
> -qalias=noansi -qstrict -qtls=default -qlanglvl=c99vla -qlanglvl=noredefmac
> -qnortti -qnoeh -qignerrno -qarch=ppc64 -DASSERT -DTARGET_ARCH_ppc
> -DINCLUDE_SUFFIX_OS=_aix -DINCLUDE_SUFFIX_CPU=_ppc -DTARGET_COMPILER_xlc
> -DPPC64 -DHOTSPOT_LIB_ARCH='"ppc64"' -DCOMPILER1 -DCOMPILER2
> -DINCLUDE_JVMCI=0 -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/aix/vm
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/cpu/ppc/vm
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os_cpu/aix_ppc/vm
> -I/priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/gensrc
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/precompiled
> -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/prims
> -DDONT_USE_PRECOMPILED_HEADER -g -qsuppress=1540-0216 -qsuppress=1540-0198
> -qsuppress=1540-1090 -qsuppress=1540-1639 -qsuppress=1540-1088
> -qsuppress=1500-010 -O3 -qhot=level=1 -qinline -qinlglue
> -DTHIS_FILE='"os_posix.cpp"' -c -qmakedep=gcc -MF
> /priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.d
> -o /priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.o
> /priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm/os_posix.cpp
>
>  -DSUPPORTS_CLOCK_MONOTONIC is the only switch appearing twice. I'm
> baffled. Do you have any idea?
>
> Regards, Thomas
>
>
> On Thu, May 18, 2017 at 12:06 PM, Magnus Ihse Bursie <
> [hidden email]> 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.
>>
>> 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
|  
Report Content as Inappropriate

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

David Holmes
Hi Thomas,

On 19/05/2017 4:39 AM, Thomas Stüfe wrote:
> Okay, I regenerated the generated-configure.sh and the double
> definitions for -DSUPPORTS_CLOCK_MONOTONIC disappeared. So the
> generated-configure.sh you posted was just outdated.

Not sure how but as long as it is fixed. :)

> However, the debug output still appears twice: configure: No
> CLOCK_GETTIME_IN_LIBRT

Yes that's a side-effect of the flag helper routine being called twice:
once for build platform and once for target.

> AC_MSG_NOTICE([No CLOCK_GETTIME_IN_LIBRT]) expands to two invocations of
> ac_echo(..). I am out of my depth here, not sure what the background is.
> But as you want to remove the debug output anyway, I think this is not
> an issue.

Right the messages will be gone.

> I will take more time for a full review later. Just adding that I really
> like this fix, it takes a lot of coding off our (platform specific)
> backs, which is a good thing!

Thanks for looking at this in so much detail already.

Cheers,
David

> Kind Regards, Thomas
>
> On Thu, May 18, 2017 at 4:40 PM, Thomas Stüfe <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hi David, Magnus,
>
>     compiles and works fine on AIX, but as mentioned before off-list to
>     David I see this stdout:
>
>     configure: No CLOCK_GETTIME_IN_LIBRT
>     configure: No CLOCK_GETTIME_IN_LIBRT
>
>     Also, the -DSUPPORTS_CLOCK_MONOTONIC appears twice on the command
>     line. Full compile command looks like this:
>
>     /bin/xlC_r -q64 -qpic -D_REENTRANT -D__STDC_FORMAT_MACROS
>     -DSUPPORTS_CLOCK_MONOTONIC -DSUPPORTS_CLOCK_MONOTONIC -DAIX
>     -qtune=balanced -qalias=noansi -qstrict -qtls=default
>     -qlanglvl=c99vla -qlanglvl=noredefmac -qnortti -qnoeh -qignerrno
>     -qarch=ppc64 -DASSERT -DTARGET_ARCH_ppc -DINCLUDE_SUFFIX_OS=_aix
>     -DINCLUDE_SUFFIX_CPU=_ppc -DTARGET_COMPILER_xlc -DPPC64
>     -DHOTSPOT_LIB_ARCH='"ppc64"' -DCOMPILER1 -DCOMPILER2
>     -DINCLUDE_JVMCI=0
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/aix/vm
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/cpu/ppc/vm
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os_cpu/aix_ppc/vm -I/priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/gensrc
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/precompiled
>     -I/priv/d031900/openjdk/jdk10-hs/source/hotspot/src/share/vm/prims
>     -DDONT_USE_PRECOMPILED_HEADER -g -qsuppress=1540-0216
>     -qsuppress=1540-0198 -qsuppress=1540-1090 -qsuppress=1540-1639
>     -qsuppress=1540-1088 -qsuppress=1500-010 -O3 -qhot=level=1 -qinline
>     -qinlglue -DTHIS_FILE='"os_posix.cpp"' -c -qmakedep=gcc -MF
>     /priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.d
>     -o
>     /priv/d031900/openjdk/jdk10-hs/output/hotspot/variant-server/libjvm/objs/os_posix.o
>     /priv/d031900/openjdk/jdk10-hs/source/hotspot/src/os/posix/vm/os_posix.cpp
>
>      -DSUPPORTS_CLOCK_MONOTONIC is the only switch appearing twice. I'm
>     baffled. Do you have any idea?
>
>     Regards, Thomas
>
>
>     On Thu, May 18, 2017 at 12:06 PM, Magnus Ihse Bursie
>     <[hidden email]
>     <mailto:[hidden email]>> 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
>                     <https://bugs.openjdk.java.net/browse/JDK-8174231>
>
>                     webrevs:
>
>                     Build-related:
>                     http://cr.openjdk.java.net/~dholmes/8174231/webrev.top/
>                     <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.
>
>         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/
>                     <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
|  
Report Content as Inappropriate

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

David Holmes
In reply to this post by Magnus Ihse Bursie
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
|  
Report Content as Inappropriate

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

Erik Joelsson
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
|  
Report Content as Inappropriate

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

Robbin Ehn
In reply to this post by David Holmes
Hi David,

On 05/18/2017 08: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/
>        hotspot: http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/
>

I like this, with neg delta of 700 loc, nice!

It's hard to see if you broken anything, since you combined 4 separate implementation into 1.
I guess you have tested this proper?

What stands out in os_posix.cpp is the
static void to_abstime(timespec* abstime, jlong timeout, bool isAbsolute)

The ifdef scopes of SUPPORTS_CLOCK_MONOTONIC is large and calculations are repeated 3 times.

Please consider something like:

#ifdef SUPPORTS_CLOCK_MONOTONIC
   if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we using this when not isAbsolute is set?
                                                       // I suggest removing that check from this if and use monotonic for that also.
      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
|  
Report Content as Inappropriate

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

David Holmes
Hi Robbin,

Thanks for looking at this.

On 19/05/2017 6:36 PM, Robbin Ehn wrote:

> Hi David,
>
> On 05/18/2017 08: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/
>>        hotspot:
>> http://cr.openjdk.java.net/~dholmes/8174231/webrev.hotspot/
>>
>
> I like this, with neg delta of 700 loc, nice!
>
> It's hard to see if you broken anything, since you combined 4 separate
> implementation into 1.

Well not really. As I said this is basically a cleaned up version of the
Linux code. The BSD and AIX versions were already based on earlier
versions of the Linux code, minus the proper handling of CLOCK_MONOTONIC
and absolute timeouts.

> I guess you have tested this proper?

Only JPRT so far. I should have mentioned that I'm not expecting this to
be reviewed in pushed within a couple of days, so some refinements and
continual testing will occur.

> What stands out in os_posix.cpp is the
> static void to_abstime(timespec* abstime, jlong timeout, bool isAbsolute)
>
> The ifdef scopes of SUPPORTS_CLOCK_MONOTONIC is large and calculations
> are repeated 3 times.

They have to be as there are three cases:

1. Relative wait using CLOCK_MONOTONIC
2. Relative wait using gettimeofday()
3. Absolute wait using gettimeofday()

> Please consider something like:
>
> #ifdef SUPPORTS_CLOCK_MONOTONIC
>    if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we
> using this when not isAbsolute is set?
>                                 // I suggest removing that check from
> this if and use monotonic for that also.

Absolute waits have to be based on wall-clock time and follow any
adjustments made to wall clock time. In contrast relative waits should
never be affected by wall-clock time adjustments hence the use of
CLOCK_MONOTONIC when available.

In Java the relative timed-waits are:
- Thread.sleep(ms)
- Object.wait(ms)/wait(ms,ns)
- LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built on
top of it)

While the only absolute timed-wait we have is the LockSupport.parkUntil
method(s).

Hope that clarifies things.

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
|  
Report Content as Inappropriate

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

Magnus Ihse Bursie
In reply to this post by David Holmes

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.
Code looks good!

In the future, I very much prefer if you do not update webrevs in place.
It's hopeless if you start reading a thread after some updates have
occured, the mails don't make any sense, and it's hard to follow
after-the-fact how the patch evolved.

>
> 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!
That's good to know.

/Magnus

>
> 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
|  
Report Content as Inappropriate

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

David Holmes
Replying to all this time :)

On 19/05/2017 7:15 PM, Magnus Ihse Bursie wrote:

>
> 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.
> Code looks good!

Thanks for the re-review.

> In the future, I very much prefer if you do not update webrevs in place.
> It's hopeless if you start reading a thread after some updates have
> occured, the mails don't make any sense, and it's hard to follow
> after-the-fact how the patch evolved.

Sorry. Point taken.

David
-----


>>
>> 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!
> That's good to know.
>
> /Magnus
>>
>> 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
|  
Report Content as Inappropriate

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

Doug Simon @ Oracle
In reply to this post by Magnus Ihse Bursie

> On 19 May 2017, at 11:15, Magnus Ihse Bursie <[hidden email]> wrote:
>
>
> 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.
> Code looks good!
>
> In the future, I very much prefer if you do not update webrevs in place. It's hopeless if you start reading a thread after some updates have occured, the mails don't make any sense, and it's hard to follow after-the-fact how the patch evolved.

Is there any chance openjdk code reviewing will adopt a slightly more modern process than webrevs such as Crucible where a full history of code evolution during a review is preserved?

-Doug
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Robbin Ehn
In reply to this post by David Holmes
On 05/19/2017 11:07 AM, David Holmes wrote:

>
> They have to be as there are three cases:
>
> 1. Relative wait using CLOCK_MONOTONIC
> 2. Relative wait using gettimeofday()
> 3. Absolute wait using gettimeofday()
>
>> Please consider something like:
>>
>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>    if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't we using this when not isAbsolute is set?
>>                                 // I suggest removing that check from this if and use monotonic for that also.
>
> Absolute waits have to be based on wall-clock time and follow any adjustments made to wall clock time. In contrast relative waits should never be affected by wall-clock
> time adjustments hence the use of CLOCK_MONOTONIC when available.
>
> In Java the relative timed-waits are:
> - Thread.sleep(ms)
> - Object.wait(ms)/wait(ms,ns)
> - LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built on top of it)
>
> While the only absolute timed-wait we have is the LockSupport.parkUntil method(s).
>
> Hope that clarifies things.

Yes thanks!

But you can still re-factoring to something similar to what I suggested and two of the calculation should be the same just ns vs us, correct?
Leaving the if statement with the "!isAbsolute" check, in my head calc_time is something like:

void calc_time(...) {
   if (isAbsolute) {
     calc_abs_time(...);
   } else {
     calc_rel_time(...);
   }
}

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
|  
Report Content as Inappropriate

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

David Holmes
On 19/05/2017 7:25 PM, Robbin Ehn wrote:

> On 05/19/2017 11:07 AM, David Holmes wrote:
>>
>> They have to be as there are three cases:
>>
>> 1. Relative wait using CLOCK_MONOTONIC
>> 2. Relative wait using gettimeofday()
>> 3. Absolute wait using gettimeofday()
>>
>>> Please consider something like:
>>>
>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>    if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why aren't
>>> we using this when not isAbsolute is set?
>>>                                 // I suggest removing that check from
>>> this if and use monotonic for that also.
>>
>> Absolute waits have to be based on wall-clock time and follow any
>> adjustments made to wall clock time. In contrast relative waits should
>> never be affected by wall-clock time adjustments hence the use of
>> CLOCK_MONOTONIC when available.
>>
>> In Java the relative timed-waits are:
>> - Thread.sleep(ms)
>> - Object.wait(ms)/wait(ms,ns)
>> - LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built
>> on top of it)
>>
>> While the only absolute timed-wait we have is the
>> LockSupport.parkUntil method(s).
>>
>> Hope that clarifies things.
>
> Yes thanks!
>
> But you can still re-factoring to something similar to what I suggested
> and two of the calculation should be the same just ns vs us, correct?

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.

> Leaving the if statement with the "!isAbsolute" check, in my head
> calc_time is something like:
>
> void calc_time(...) {
>    if (isAbsolute) {
>      calc_abs_time(...);
>    } else {
#ifdef SUPPORTS_CLOCK_MONOTONIC
>      calc_rel_time_from_clock_monotonic(...);
#else
 >      calc_rel_time_from_gettimeofday(...);
#endif
>    }
> }

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
|  
Report Content as Inappropriate

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

David Holmes
Correction ...

On 19/05/2017 8:53 PM, David Holmes wrote:

> On 19/05/2017 7:25 PM, Robbin Ehn wrote:
>> On 05/19/2017 11:07 AM, David Holmes wrote:
>>>
>>> They have to be as there are three cases:
>>>
>>> 1. Relative wait using CLOCK_MONOTONIC
>>> 2. Relative wait using gettimeofday()
>>> 3. Absolute wait using gettimeofday()
>>>
>>>> Please consider something like:
>>>>
>>>> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>>>    if (_use_clock_monotonic_condattr  && !isAbsolute) { // Why
>>>> aren't we using this when not isAbsolute is set?
>>>>                                 // I suggest removing that check
>>>> from this if and use monotonic for that also.
>>>
>>> Absolute waits have to be based on wall-clock time and follow any
>>> adjustments made to wall clock time. In contrast relative waits
>>> should never be affected by wall-clock time adjustments hence the use
>>> of CLOCK_MONOTONIC when available.
>>>
>>> In Java the relative timed-waits are:
>>> - Thread.sleep(ms)
>>> - Object.wait(ms)/wait(ms,ns)
>>> - LockSupport.parkNanos(ns) (and all the j.u.c blocking methods built
>>> on top of it)
>>>
>>> While the only absolute timed-wait we have is the
>>> LockSupport.parkUntil method(s).
>>>
>>> Hope that clarifies things.
>>
>> Yes thanks!
>>
>> But you can still re-factoring to something similar to what I
>> suggested and two of the calculation should be the same just ns vs us,
>> correct?
>
> 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.
>
>> Leaving the if statement with the "!isAbsolute" check, in my head
>> calc_time is something like:
>>
>> void calc_time(...) {
>>    if (isAbsolute) {
>>      calc_abs_time(...);
>>    } else {
> #ifdef SUPPORTS_CLOCK_MONOTONIC
>>      calc_rel_time_from_clock_monotonic(...);
> #else
>  >      calc_rel_time_from_gettimeofday(...);
> #endif
>>    }
>> }

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.

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
|  
Report Content as Inappropriate

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

Robbin Ehn
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/

This makes the calculation independent of source/unit.

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