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

David Holmes david.holmes at oracle.com
Tue May 30 20:50:51 UTC 2017

Hi Dan,

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

On 5/28/17 10:19 PM, David Holmes wrote:

Dan, Robbin, Thomas,

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

Okay - thanks again.

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

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

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

Right this is single-threaded VM init.

An alternative would be to set useclockmonotoniccondattr to true only when pthreadcondattrsetclock() returns 0.

Yes - fixed.

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

Fixed.

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

Fixed.

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

Fixed.

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

src/os/aix/vm/osaix.cpp No comments. src/os/bsd/vm/osbsd.hpp No comments. src/os/bsd/vm/osbsd.cpp No comments. src/os/linux/vm/oslinux.hpp No comments. src/os/linux/vm/oslinux.cpp No comments. src/os/solaris/vm/ossolaris.hpp No comments. src/os/solaris/vm/ossolaris.cpp No comments.

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

Thanks again.

David

Dan

this fixes all Dan's nits and refactors the time calculation code as suggested by Robbin. Thomas: if you are around and able, it would be good to get a final sanity check on AIX. Thanks. Testing: - JPRT: -testset hotspot -testset core - manual: - jtreg:java/util/concurrent - various little test programs that try to validate sleep/wait times to show early returns or unexpected delays Thanks again for the reviews. David On 29/05/2017 10:29 AM, David Holmes wrote: On 27/05/2017 4:19 AM, Daniel D. Daugherty wrote: On 5/26/17 1:27 AM, David Holmes wrote: Robbin, Dan,

Below is a modified version of the refactored toabstime 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 "nowpartsec" is - nanos or micros. And the calcabstime always has a deadline in millis. So I simplified and did a little renaming, and tracked maxsecs in debugonly 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 "nowpartsec" (may be nanos or micros depending // on which clock is being used). static void calcreltime(timespec* abstime, jlong timeout, jlong nowsec, jlong nowpartsec, jlong unit) { timet maxsecs = nowsec + MAXSECS; jlong seconds = timeout / NANOUNITS; timeout %= NANOUNITS; // remaining nanos if (seconds >= MAXSECS) { // More seconds than we can add, so pin to maxsecs. abstime->tvsec = maxsecs; abstime->tvnsec = 0; } else { abstime->tvsec = nowsec + seconds; long nanos = (nowpartsec * (NANOUNITS / unit)) + timeout; if (nanos >= NANOUNITS) { // overflow abstime->tvsec += 1; nanos -= NANOUNITS; } abstime->tvnsec = 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 unpackabstime(timespec* abstime, jlong deadline, jlong nowsec) { timet maxsecs = nowsec + MAXSECS; jlong seconds = deadline / MILLIUNITS; jlong millis = deadline % MILLIUNITS; if (seconds >= maxsecs) { // Absolute seconds exceeds allowed max, so pin to maxsecs. abstime->tvsec = maxsecs; abstime->tvnsec = 0; } else { abstime->tvsec = seconds; abstime->tvnsec = millis * (NANOUNITS / MILLIUNITS); } } static void toabstime(timespec* abstime, jlong timeout, bool isAbsolute) { There's an extra blank line here. Fixed. DEBUGONLY(int maxsecs = MAXSECS;) if (timeout < 0) {_ _timeout = 0;_ _}_ _#ifdef SUPPORTSCLOCKMONOTONIC_ _if (useclockmonotoniccondattr && !isAbsolute) {_ _struct timespec now;_ _int status = clockgettime(CLOCKMONOTONIC, &now);_ _assertstatus(status == 0, status, "clockgettime");_ _calcreltime(abstime, timeout, now.tvsec, now.tvnsec,_ _NANOUNITS);_ _DEBUGONLY(maxsecs += now.tvsec;)_ _} else {_ _#else_ _{ // Match the block scope._ _#endif // SUPPORTSCLOCKMONOTONIC_ _// Time-of-day clock is all we can reliably use._ _struct timeval now;_ _int status = gettimeofday(&now, NULL);_ _assert(status == 0, "gettimeofday");_ _assertstatus() is used above, but assert() is used here. Why?_ _Historical. assertstatus 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_ _assertstatus with these and pass errno as the "status". So I did that._ _if (isAbsolute) {_ _unpackabstime(abstime, timeout, now.tvsec);_ _}_ _else {_ _Inconsistent "else-branch" formatting._ _I believe HotSpot style is "} else {"_ _Fixed._ _calcreltime(abstime, timeout, now.tvsec, now.tvusec, MICROUNITS);_ _}_ _DEBUGONLY(maxsecs += now.tvsec;)_ _}_ _assert(abstime->tvsec >= 0, "tvsec < 0");_ _assert(abstime->tvsec <= maxsecs, "tvsec > maxsecs"); assert(abstime->tvnsec >= 0, "tvnsec < 0");_ _assert(abstime->tvnsec < NANOSECSPERSEC, "tvnsec >= nanospersec"); Why does the assert mesg have "nanospersec" instead of "NANOSECSPERSEC"? No reason. Actually that should now refer to NANOUNITS. Hmmm I can not recall why we have NANOUNITS and NANAOSECSPERSEC ... 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