(10) (M) RFR: 8174231: Factor out and share PlatformEvent and Parker code for POSIX systems
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue May 30 13:58:27 UTC 2017
On 5/30/17 6:20 AM, Thomas Stüfe wrote:
> 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?
Just starting my re-review now, but I'm inclined to agree os_posix.hpp
is not meant to be a self-contained header...
Dan
>>
>> 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