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