(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems (original) (raw)
Thomas Stüfe thomas.stuefe at gmail.com
Tue May 30 09:53:39 UTC 2017
- Previous message: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
- Next message: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Hi David,
works fine on AIX, as it did before.
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?
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.
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> wrote:
Dan, Robbin, Thomas,
Okay here is the final ready to push version: 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 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
- Previous message: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
- Next message: (10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]