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
