(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:28:25 UTC 2017
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 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.
So ... os_posix.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.
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 (*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