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

2017-05-25 Thread Daniel D. Daugherty 

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, ) == 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

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

2017-05-19 Thread 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, );
  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(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
   }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread 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, );
  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(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_usec, MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread 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, );
  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(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, 
now.tv_usec, MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread Robbin Ehn 

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, );
  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(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
   }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread Doug Simon 

> On 19 May 2017, at 11:15, 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!
> 
> 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

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

2017-05-19 Thread 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

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

2017-05-19 Thread Magnus Ihse Bursie 


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

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

2017-05-19 Thread 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, );
  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(, NULL);
  assert(status == 0, "gettimeofday");
  calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);

   }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread 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

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

2017-05-19 Thread Robbin Ehn 

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, );
 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(, NULL);
 assert(status == 0, "gettimeofday");
 calc_time(abstime, timeout, isAbsolute, now.tv_sec, now.tv_usec, 
MICROUNITS);
  }
#endif

Thanks for fixing this!

/Robbin

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

2017-05-19 Thread 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

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

2017-05-19 Thread David Holmes 

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

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

2017-05-18 Thread 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 > 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
> 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

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

2017-05-18 Thread Thomas Stüfe 
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 
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 <
> magnus.ihse.bur...@oracle.com> 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

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

2017-05-18 Thread Thomas Stüfe 
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 <
magnus.ihse.bur...@oracle.com> 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

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

2017-05-18 Thread 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

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

2017-05-18 Thread 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

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

2017-05-18 Thread 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