IMPALA-4631: don't use floating point operations for time unit conversions This was leading to the PlanFragmentExecutor::Close() DCHECK because with floating point we can have c * a + c * b > c * (a + b). Also note this is much more likely to happen when using the MONOTONIC_COARSE since that will result in the nested scoped timers ending up starting/stopping at exactly the same time. Additionally, the new code is faster.
Change-Id: I7237f579b201f5bd3930f66e9c2c8d700c37ffeb Reviewed-on: http://gerrit.cloudera.org:8080/5434 Reviewed-by: Dan Hecht <[email protected]> Tested-by: Jim Apple <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/54194af6 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/54194af6 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/54194af6 Branch: refs/heads/hadoop-next Commit: 54194af6ef08048a1ae367f29350228dafd8f2aa Parents: d6eb1b1 Author: Dan Hecht <[email protected]> Authored: Thu Dec 8 15:29:47 2016 -0800 Committer: Dan Hecht <[email protected]> Committed: Thu Dec 15 22:37:47 2016 +0000 ---------------------------------------------------------------------- be/src/gutil/walltime.cc | 12 ------------ be/src/gutil/walltime.h | 20 ++++++++++++-------- be/src/util/stopwatch.h | 2 +- be/src/util/time.h | 6 +++--- 4 files changed, 16 insertions(+), 24 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/54194af6/be/src/gutil/walltime.cc ---------------------------------------------------------------------- diff --git a/be/src/gutil/walltime.cc b/be/src/gutil/walltime.cc index b497500..04d7f4b 100644 --- a/be/src/gutil/walltime.cc +++ b/be/src/gutil/walltime.cc @@ -166,18 +166,6 @@ bool WallTime_Parse_Timezone(const char* time_spec, return true; } -WallTime WallTime_Now() { -#if defined(__APPLE__) - mach_timespec_t ts; - walltime_internal::GetCurrentTime(&ts); - return ts.tv_sec + ts.tv_nsec / static_cast<double>(1e9); -#else - timespec ts; - clock_gettime(CLOCK_REALTIME, &ts); - return ts.tv_sec + ts.tv_nsec / static_cast<double>(1e9); -#endif // defined(__APPLE__) -} - void StringAppendStrftime(string* dst, const char* format, time_t when, http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/54194af6/be/src/gutil/walltime.h ---------------------------------------------------------------------- diff --git a/be/src/gutil/walltime.h b/be/src/gutil/walltime.h index 8d31627..2f04ebe 100644 --- a/be/src/gutil/walltime.h +++ b/be/src/gutil/walltime.h @@ -30,6 +30,12 @@ using std::string; #include "common/logging.h" #include "gutil/integral_types.h" +#define NANOS_PER_SEC 1000000000ll +#define NANOS_PER_MICRO 1000ll +#define MICROS_PER_SEC 1000000ll +#define MICROS_PER_MILLI 1000ll +#define MILLIS_PER_SEC 1000ll + typedef double WallTime; // Append result to a supplied string. @@ -52,9 +58,6 @@ bool WallTime_Parse_Timezone(const char* time_spec, bool local, WallTime* result); -// Return current time in seconds as a WallTime. -WallTime WallTime_Now(); - typedef int64 MicrosecondsInt64; typedef int64 NanosecondsInt64; @@ -76,7 +79,7 @@ inline void GetCurrentTime(mach_timespec_t* ts) { inline MicrosecondsInt64 GetCurrentTimeMicros() { mach_timespec_t ts; GetCurrentTime(&ts); - return ts.tv_sec * 1e6 + ts.tv_nsec / 1e3; + return ts.tv_sec * MICROS_PER_SEC + ts.tv_nsec / NANOS_PER_MICRO; } inline int64_t GetMonoTimeNanos() { @@ -91,7 +94,7 @@ inline int64_t GetMonoTimeNanos() { } inline MicrosecondsInt64 GetMonoTimeMicros() { - return GetMonoTimeNanos() / 1e3; + return GetMonoTimeNanos() / NANOS_PER_MICRO; } inline MicrosecondsInt64 GetThreadCpuTimeMicros() { @@ -117,7 +120,8 @@ inline MicrosecondsInt64 GetThreadCpuTimeMicros() { return 0; } - return thread_info_data.user_time.seconds * 1e6 + thread_info_data.user_time.microseconds; + return thread_info_data.user_time.seconds * MICROS_PER_SEC + + thread_info_data.user_time.microseconds; } #else @@ -125,13 +129,13 @@ inline MicrosecondsInt64 GetThreadCpuTimeMicros() { inline MicrosecondsInt64 GetClockTimeMicros(clockid_t clock) { timespec ts; clock_gettime(clock, &ts); - return ts.tv_sec * 1e6 + ts.tv_nsec / 1e3; + return ts.tv_sec * MICROS_PER_SEC + ts.tv_nsec / NANOS_PER_MICRO; } inline NanosecondsInt64 GetClockTimeNanos(clockid_t clock) { timespec ts; clock_gettime(clock, &ts); - return ts.tv_sec * 1e9 + ts.tv_nsec; + return ts.tv_sec * NANOS_PER_SEC + ts.tv_nsec; } #endif // defined(__APPLE__) http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/54194af6/be/src/util/stopwatch.h ---------------------------------------------------------------------- diff --git a/be/src/util/stopwatch.h b/be/src/util/stopwatch.h index 0e73b6a..c1f85aa 100644 --- a/be/src/util/stopwatch.h +++ b/be/src/util/stopwatch.h @@ -170,7 +170,7 @@ class MonotonicStopWatch { // Now() can be called frequently (IMPALA-2407). timespec ts; clock_gettime(OsInfo::fast_clock(), &ts); - return ts.tv_sec * 1e9 + ts.tv_nsec; + return ts.tv_sec * NANOS_PER_SEC + ts.tv_nsec; #endif } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/54194af6/be/src/util/time.h ---------------------------------------------------------------------- diff --git a/be/src/util/time.h b/be/src/util/time.h index 28a0f66..efe6a3b 100644 --- a/be/src/util/time.h +++ b/be/src/util/time.h @@ -39,11 +39,11 @@ inline int64_t MonotonicMicros() { // 63 bits ~= 5K years uptime } inline int64_t MonotonicMillis() { - return GetMonoTimeMicros() / 1e3; + return GetMonoTimeMicros() / MICROS_PER_MILLI; } inline int64_t MonotonicSeconds() { - return GetMonoTimeMicros() / 1e6; + return GetMonoTimeMicros() / MICROS_PER_SEC; } @@ -52,7 +52,7 @@ inline int64_t MonotonicSeconds() { /// a cluster. For more accurate timings on the local host use the monotonic functions /// above. inline int64_t UnixMillis() { - return GetCurrentTimeMicros() / 1e3; + return GetCurrentTimeMicros() / MICROS_PER_MILLI; } /// Sleeps the current thread for at least duration_ms milliseconds.
