(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 12:20:53 UTC 2017

On Tue, May 30, 2017 at 1:28 PM, David Holmes <david.holmes at oracle.com> wrote:

Correction ...

On 30/05/2017 9:22 PM, David Holmes wrote:

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 osposix.hpp (pthreadmutext, pthreadcondt). Are the rules not that each header should be self-contained? If yes, should osposix.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. So ... osposix.hpp only #includes "runtime/os.hpp" and yet uses dozens of types defined in numerous other header files! So I could add pthread.h, but it would look very lonely. Looking more closely, this seems not to be a real header, similar to all the other os_xxx.hpp files, but its only purpose seems to be to get included by os.hpp at that one specific place. So, the rules to be self-contained does not apply, right?

Which also means the include of runtime/os.hpp is a bit pointless.

..Thomas

Dan: thoughts?

Cheers, David

- 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 (*clockgetresfunct)(clockidt, struct timespec *); ... static clockgetresfunct clockgetresfunc = (clockgetresfunct) 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 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