(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
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 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.
>
>
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 (*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